| Message ID | 20171019081847.16171-4-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | libnftables preparations | expand |
On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote: > This allows an application to explicitly flush caches associated with a > given nft context. > > Note that this is a bit inconsistent in that it releases the global > interface cache, but nft_ctx_free() does the same so at least it's not a > regression. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > include/nftables/nftables.h | 1 + > src/libnftables.c | 9 +++++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h > index 052a77bfb5371..fbc6fd4252a97 100644 > --- a/include/nftables/nftables.h > +++ b/include/nftables/nftables.h > @@ -77,6 +77,7 @@ enum nftables_exit_codes { > struct nft_ctx *nft_ctx_new(uint32_t flags); > void nft_ctx_free(struct nft_ctx *ctx); > FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); > +void nft_ctx_flush_cache(struct nft_ctx *ctx); > > int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock, > void *scanner, struct parser_state *state, > diff --git a/src/libnftables.c b/src/libnftables.c > index 187747c66af21..0de50c854d572 100644 > --- a/src/libnftables.c > +++ b/src/libnftables.c > @@ -146,13 +146,18 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) > return ctx; > } > > +void nft_ctx_flush_cache(struct nft_ctx *ctx) > +{ > + iface_cache_release(); > + cache_release(&ctx->cache); > +} This flush allows us to release the cache, but nft_ctx_alloc() populates it. I'm missing something here, can we force a context repopulation? If there is no usecase for this yet, I would keep this behind by now. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Oct 20, 2017 at 02:13:26PM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote: [...] > > +void nft_ctx_flush_cache(struct nft_ctx *ctx) > > +{ > > + iface_cache_release(); > > + cache_release(&ctx->cache); > > +} > > This flush allows us to release the cache, but nft_ctx_alloc() > populates it. I'm missing something here, can we force a context > repopulation? No, nft_ctx_alloc() does not populate the cache, but just initialize cache list head (which is not undone by cache_release()). Cache population happens during command execution depending on whether a cache is needed or not. > If there is no usecase for this yet, I would keep this behind by now. The use-case for the above is cli_complete(), which explicitly drops the cache after execution of every command (probably because it's potentially long-lived and therefore things might change in background). Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 20, 2017 at 07:05:13PM +0200, Phil Sutter wrote: > Hi, > > On Fri, Oct 20, 2017 at 02:13:26PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote: > [...] > > > +void nft_ctx_flush_cache(struct nft_ctx *ctx) > > > +{ > > > + iface_cache_release(); > > > + cache_release(&ctx->cache); > > > +} > > > > This flush allows us to release the cache, but nft_ctx_alloc() > > populates it. I'm missing something here, can we force a context > > repopulation? > > No, nft_ctx_alloc() does not populate the cache, but just initialize > cache list head (which is not undone by cache_release()). Cache > population happens during command execution depending on whether a cache > is needed or not. I see. I think cache population should happen from nft_ctx_alloc(), caches are context after all. > > If there is no usecase for this yet, I would keep this behind by now. > > The use-case for the above is cli_complete(), which > explicitly drops the cache after execution of every command (probably > because it's potentially long-lived and therefore things might change in > background). I see. If we follow the approach I'm describe above, then we need something like nft_ctx_reset(), where we reset all context and we get a fresh cache. Makes sense to you? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 20, 2017 at 09:10:31PM +0200, Pablo Neira Ayuso wrote: > On Fri, Oct 20, 2017 at 07:05:13PM +0200, Phil Sutter wrote: > > Hi, > > > > On Fri, Oct 20, 2017 at 02:13:26PM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote: > > [...] > > > > +void nft_ctx_flush_cache(struct nft_ctx *ctx) > > > > +{ > > > > + iface_cache_release(); > > > > + cache_release(&ctx->cache); > > > > +} > > > > > > This flush allows us to release the cache, but nft_ctx_alloc() > > > populates it. I'm missing something here, can we force a context > > > repopulation? > > > > No, nft_ctx_alloc() does not populate the cache, but just initialize > > cache list head (which is not undone by cache_release()). Cache > > population happens during command execution depending on whether a cache > > is needed or not. > > I see. > > I think cache population should happen from nft_ctx_alloc(), caches > are context after all. > > > > If there is no usecase for this yet, I would keep this behind by now. > > > > The use-case for the above is cli_complete(), which > > explicitly drops the cache after execution of every command (probably > > because it's potentially long-lived and therefore things might change in > > background). > > I see. If we follow the approach I'm describe above, then we need > something like nft_ctx_reset(), where we reset all context and we get > a fresh cache. I think it's worth keeping the current logic of when to populate the cache. Remember when I added a call to cache_update() for echo output support which caused an unnecessary slowdown you later complained about? Populating the cache unconditionally upon context creation would cause the same issue again if we follow my plan of making 'nft' binary the first user of libnftables. Right now, we only explicitly clear the cache in CLI, and my patch allows applications to do that if they think it's necessary. Cache population is completely pointless if an application just wants to e.g. add a new table to the rule set, so I'd prefer to leave it this way. We spoke about possible performance implications of cache updates at NFWS already, keeping the existing (and well defined) logic will at least limit the impact of this potential bottleneck. Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h index 052a77bfb5371..fbc6fd4252a97 100644 --- a/include/nftables/nftables.h +++ b/include/nftables/nftables.h @@ -77,6 +77,7 @@ enum nftables_exit_codes { struct nft_ctx *nft_ctx_new(uint32_t flags); void nft_ctx_free(struct nft_ctx *ctx); FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); +void nft_ctx_flush_cache(struct nft_ctx *ctx); int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock, void *scanner, struct parser_state *state, diff --git a/src/libnftables.c b/src/libnftables.c index 187747c66af21..0de50c854d572 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -146,13 +146,18 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) return ctx; } +void nft_ctx_flush_cache(struct nft_ctx *ctx) +{ + iface_cache_release(); + cache_release(&ctx->cache); +} + void nft_ctx_free(struct nft_ctx *ctx) { if (ctx->nf_sock) netlink_close_sock(ctx->nf_sock); - iface_cache_release(); - cache_release(&ctx->cache); + nft_ctx_flush_cache(ctx); xfree(ctx); nft_exit(); }
This allows an application to explicitly flush caches associated with a given nft context. Note that this is a bit inconsistent in that it releases the global interface cache, but nft_ctx_free() does the same so at least it's not a regression. Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/nftables/nftables.h | 1 + src/libnftables.c | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)