Re: [PATCH nftables 6/6] src: add trace support to nft monitor mode

[Patrick McHardy <kaber@trash.net>]

On 24.11, Pablo Neira Ayuso wrote:
> On Tue, Nov 24, 2015 at 11:04:29AM +0000, Patrick McHardy wrote:
> > On 24.11, Pablo Neira Ayuso wrote:
> > Well I keep running into problems with it. We already discussed a few, we're
> > dumping way to much information that we don't need and we're making nft
> > require root even for unpriviledged operations and just testing ruleset
> > syntax.
> > 
> > We're basing errors on a cache that might not be up to date.
> > 
> > When I list the bridge table, for some reason it lists *all* tables of all
> > families. We're basically doing full dumps of everything in many cases.
> > This will be absolutely killing performance with a big ruleset.
> 
> OK, I'll be refining this to allow selective dumps.
> 
> > AFAIK (did not test) we're only listing sets for the family of the first
> > command, then set cache_initializer to true and skip further updates. When
> > the ruleset refers to multiple families, the contents will not be present
> > but expected.
> 
> This may be related to the kernel patches I have to send to use
> generations from the dump path.

As mentioned, I did not actually test this so far, but its the caching
code, it initializes for the first rule and only for that rule and bases
its queries on that handle. OTOH the dumping of all tables indicates that
it *does* dump more than that. Not sure, would have to test to be sure,
but the code looks like it.

> > It basically seems like the big hammer approach + some bugs instead of
> > selectively getting what we need when we need it and making sure its
> > up to date, at least before generating errors based on it.
> 
> Selective dumping is good to have indeed, I'm willing to refine this.
> 
> But regarding event tracing, I think it's good to keep a cache in
> userspace that we update based on the events that we receive, then
> resync if we hit ENOBUFS, instead of inquiring the kernel for every
> trace.

Generally I agree.

> So I think Florian can get this in. I'll be resolving the existing
> caching issues after him as this is not related to his work.
> 
> Fine with this approach?

Sure, it we fix this, no question :)