| Message ID | 20240815113712.1266545-1-pablo@netfilter.org |
|---|---|
| Headers | show |
| Series | relax cache requirements, speed up incremental updates | expand |
On Thu, Aug 15, 2024 at 01:37:07PM +0200, Pablo Neira Ayuso wrote: > Hi, > > The following patchset relaxes cache requirements, this is based on the > observation that objects are fetched to report errors and provide hints. This is nice as it applies to error path only, though the second cache fetch is prone to race conditions. Did you consider retrying the whole transaction with beefed-up cache in error case? I was about to mention how it nicely integrates with transaction refresh in ERESTART case, but then realized this is iptables code and nft doesn't retry in that case?! Cheers, Phil
On Thu, Aug 15, 2024 at 02:25:15PM +0200, Phil Sutter wrote: > On Thu, Aug 15, 2024 at 01:37:07PM +0200, Pablo Neira Ayuso wrote: > > Hi, > > > > The following patchset relaxes cache requirements, this is based on the > > observation that objects are fetched to report errors and provide hints. > > This is nice as it applies to error path only, though the second cache > fetch is prone to race conditions. The call to nft_cache_update() ensures cache is consistent, old cache is dropped and a new consistent cache is obtained. The hint could be misleading (worst case) though since the cache could have different generation ID that the transaction itself, but it is just a hint. > Did you consider retrying the whole transaction with beefed-up cache > in error case? Why retry? I am assuming a batch where the user made a mistake, retry will fail again. > I was about to mention how it nicely integrates with transaction > refresh in ERESTART case, but then realized this is iptables code > and nft doesn't retry in that case?! I think you are talking about different scenario, that is, userspace sends an update but generation ID mismatches, kernel reports ERESTART and nftables revamps, this is to catch an interference with another process, that needs to be done in nft, but it is a different issue.
On Thu, Aug 15, 2024 at 02:46:02PM +0200, Pablo Neira Ayuso wrote: > On Thu, Aug 15, 2024 at 02:25:15PM +0200, Phil Sutter wrote: > > On Thu, Aug 15, 2024 at 01:37:07PM +0200, Pablo Neira Ayuso wrote: > > > Hi, > > > > > > The following patchset relaxes cache requirements, this is based on the > > > observation that objects are fetched to report errors and provide hints. > > > > This is nice as it applies to error path only, though the second cache > > fetch is prone to race conditions. > > The call to nft_cache_update() ensures cache is consistent, old cache > is dropped and a new consistent cache is obtained. The hint could be > misleading (worst case) though since the cache could have different > generation ID that the transaction itself, but it is just a hint. > > > Did you consider retrying the whole transaction with beefed-up cache > > in error case? > > Why retry? I am assuming a batch where the user made a mistake, retry > will fail again. > > > I was about to mention how it nicely integrates with transaction > > refresh in ERESTART case, but then realized this is iptables code > > and nft doesn't retry in that case?! > > I think you are talking about different scenario, that is, userspace > sends an update but generation ID mismatches, kernel reports ERESTART > and nftables revamps, this is to catch an interference with another > process, that needs to be done in nft, but it is a different issue. Yes, I had incorrect error reporting in mind: Kernel reports ENOENT for a chain which another process creates concurrently. The error path cache update fetches the newly created chain and error reporting suggests to use the exact chain user specified (I assume). It is indeed a corner-case issue, though. Cheers, Phil
On Thu, Aug 15, 2024 at 03:10:13PM +0200, Phil Sutter wrote: > On Thu, Aug 15, 2024 at 02:46:02PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Aug 15, 2024 at 02:25:15PM +0200, Phil Sutter wrote: > > > On Thu, Aug 15, 2024 at 01:37:07PM +0200, Pablo Neira Ayuso wrote: > > > > Hi, > > > > > > > > The following patchset relaxes cache requirements, this is based on the > > > > observation that objects are fetched to report errors and provide hints. > > > > > > This is nice as it applies to error path only, though the second cache > > > fetch is prone to race conditions. > > > > The call to nft_cache_update() ensures cache is consistent, old cache > > is dropped and a new consistent cache is obtained. The hint could be > > misleading (worst case) though since the cache could have different > > generation ID that the transaction itself, but it is just a hint. > > > > > Did you consider retrying the whole transaction with beefed-up cache > > > in error case? > > > > Why retry? I am assuming a batch where the user made a mistake, retry > > will fail again. > > > > > I was about to mention how it nicely integrates with transaction > > > refresh in ERESTART case, but then realized this is iptables code > > > and nft doesn't retry in that case?! > > > > I think you are talking about different scenario, that is, userspace > > sends an update but generation ID mismatches, kernel reports ERESTART > > and nftables revamps, this is to catch an interference with another > > process, that needs to be done in nft, but it is a different issue. > > Yes, I had incorrect error reporting in mind: Kernel reports ENOENT for > a chain which another process creates concurrently. The error path cache > update fetches the newly created chain and error reporting suggests to > use the exact chain user specified (I assume). IIRC, the fuzzy match code skips exact matches, worst case can be a very hint. > It is indeed a corner-case issue, though. ERESTART handling can be useful for your rule index feature, where consistency is fundamental to ensure that rule is added where the user really wants.
On Thu, Aug 15, 2024 at 01:37:07PM +0200, Pablo Neira Ayuso wrote: > Hi, > > The following patchset relaxes cache requirements, this is based on the > observation that objects are fetched to report errors and provide hints. > > This is a new attempt to speed up incremental updates following a > different approach, after reverting: > > e791dbe109b6 ("cache: recycle existing cache with incremental updates") > > which is fragile because cache consistency checking needs more, it should > be still possible to explore in the future, but this seems a more simple > approach at this stage. > > This is passing tests/shell and tests/py. > > Pablo Neira Ayuso (5): > cache: rule by index requires full cache > cache: populate chains on demand from error path > cache: populate objecs on demand from error path > cache: populate flowtable on demand from error path > cache: do not fetch set inconditionally on delete > > include/cache.h | 1 - > src/cache.c | 23 ++++++----------------- > src/cmd.c | 23 +++++++++++++++++++++++ > 3 files changed, 29 insertions(+), 18 deletions(-) I applied this series to nft master and tested it against the latest net-next and RHEL-9 kernels. No issues or regressions found. Thanks Pablo! Tested-by: Eric Garver <eric@garver.life>
On Thu, Aug 15, 2024 at 11:08:58AM -0400, Eric Garver wrote: > On Thu, Aug 15, 2024 at 01:37:07PM +0200, Pablo Neira Ayuso wrote: > > Hi, > > > > The following patchset relaxes cache requirements, this is based on the > > observation that objects are fetched to report errors and provide hints. > > > > This is a new attempt to speed up incremental updates following a > > different approach, after reverting: > > > > e791dbe109b6 ("cache: recycle existing cache with incremental updates") > > > > which is fragile because cache consistency checking needs more, it should > > be still possible to explore in the future, but this seems a more simple > > approach at this stage. > > > > This is passing tests/shell and tests/py. > > > > Pablo Neira Ayuso (5): > > cache: rule by index requires full cache > > cache: populate chains on demand from error path > > cache: populate objecs on demand from error path > > cache: populate flowtable on demand from error path > > cache: do not fetch set inconditionally on delete > > > > include/cache.h | 1 - > > src/cache.c | 23 ++++++----------------- > > src/cmd.c | 23 +++++++++++++++++++++++ > > 3 files changed, 29 insertions(+), 18 deletions(-) > > I applied this series to nft master and tested it against the latest > net-next and RHEL-9 kernels. No issues or regressions found. Pushed out, thanks for testing.
Hi, The following patchset relaxes cache requirements, this is based on the observation that objects are fetched to report errors and provide hints. This is a new attempt to speed up incremental updates following a different approach, after reverting: e791dbe109b6 ("cache: recycle existing cache with incremental updates") which is fragile because cache consistency checking needs more, it should be still possible to explore in the future, but this seems a more simple approach at this stage. This is passing tests/shell and tests/py. Pablo Neira Ayuso (5): cache: rule by index requires full cache cache: populate chains on demand from error path cache: populate objecs on demand from error path cache: populate flowtable on demand from error path cache: do not fetch set inconditionally on delete include/cache.h | 1 - src/cache.c | 23 ++++++----------------- src/cmd.c | 23 +++++++++++++++++++++++ 3 files changed, 29 insertions(+), 18 deletions(-)