Re: [PATCH nft,v2] cache: do not populate the cache in case of flush ruleset command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Phil,

On Fri, Jun 14, 2019 at 03:41:24PM +0200, Phil Sutter wrote:
[...]
> On Fri, Jun 14, 2019 at 03:04:38PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jun 14, 2019 at 02:59:10PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jun 14, 2019 at 02:54:32PM +0200, Phil Sutter wrote:
> > > > Hi Pablo,
> > > > 
> > > > On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote:
> > > > > __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink
> > > > > dump to populate the cache. This patch is a workaround until we have a
> > > > > better infrastructure to track the state of the cache objects.
> > > > 
> > > > I assumed the problem wouldn't exist anymore since we're populating the
> > > > cache just once. Can you maybe elaborate a bit on the problem you're
> > > > trying to solve with that workaround?
> > > 
> > > If nft segfaults to dump the cache, 'nft flush ruleset' will not work
> > > since it always fetches the cache, it will segfault too.
> > > 
> > > The flush ruleset command was still dumping the cache before this
> > > patch.
> > 
> > In general, we still need to improve the cache logic, to make it finer
> > grain. Now that we have a single point to populate the cache, things
> > will get more simple. We need to replace the cache command level to
> > cache flags or our own cache level definitions. The existing approach
> > that uses of commands to define the cache level completeness has its
> > own limitations. We can discuss this during the NFWS :-).
> 
> Yes, I had the same thought already. It is quite unintuitive how we link
> cache completeness to certain commands. :)

Just sent a follow up patch to introduce cache level flags [1]. The
existing approach is rather conservative in what we fetch from the
kernel (sometimes more than needed) but it should be possible to
review this logic incrementally.

> Regarding your problem, maybe cache_update() should exit immediately if
> passed cmd is CMD_INVALID? Unless I miss something, if cache_evaluate()
> returns that value, we don't need a cache at all.

Problem is that CMD_INVALID does not mean empty cache.

With the new cache level infrastructure, now there is a
NFT_CACHE_EMPTY that provides the semantics this workaround was
providing in a clearer way I think.

[1] https://patchwork.ozlabs.org/patch/1116973/



[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux