netdev - Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200302113933.34fa6348@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 2 Mar 2020 11:39:33 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, saeedm@...lanox.com,
        leon@...nel.org, michael.chan@...adcom.com, vishal@...lsio.com,
        jeffrey.t.kirsher@...el.com, idosch@...lanox.com,
        aelior@...vell.com, peppe.cavallaro@...com,
        alexandre.torgue@...com, jhs@...atatu.com,
        xiyou.wangcong@...il.com, pablo@...filter.org,
        ecree@...arflare.com, mlxsw@...lanox.com
Subject: Re: [patch net-next v2 12/12] sched: act: allow user to specify
 type of HW stats for a filter

On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote:
> >> >On request:
> >> > - no attr -> any stats allowed but some stats must be provided *
> >> > - 0       -> no stats requested / disabled
> >> > - 0x1     -> must be stat type0
> >> > - 0x6     -> stat type1 or stat type2 are both fine    
> >> 
> >> I was thinking about this of course. On the write side, this is ok
> >> however, this is very tricky on read side. See below.
> >>   
> >> >* no attr kinda doesn't work 'cause u32 offload has no stats and this
> >> >  is action-level now, not flower-level :S What about u32 and matchall?    
> >> 
> >> The fact that cls does not implement stats offloading is a lack of
> >> feature of the particular cls.  
> >
> >Yeah, I wonder how that squares with strict netlink parsing.
> >  
> >> >We can add a separate attribute with "active" stat types:
> >> > - no attr -> old kernel
> >> > - 0       -> no stats are provided / stats disabled
> >> > - 0x1     -> only stat type0 is used by drivers
> >> > - 0x6     -> at least one driver is using type1 and one type2    
> >> 
> >> There are 2 problems:
> >> 1) There is a mismatch between write and read. User might pass different
> >> value than it eventually gets from kernel. I guess this might be fine.  
> >
> >Separate attribute would work.
> >  
> >> 2) Much bigger problem is, that since the same action may be offloaded
> >> by multiple drivers, the read would have to provide an array of
> >> bitfields, each array item would represent one offloaded driver. That is
> >> why I decided for simple value instead of bitfield which is the same on
> >> write and read.  
> >
> >Why an array? The counter itself is added up from all the drivers.
> >If the value is a bitfield all drivers can just OR-in their type.  
> 
> Yeah, for uapi. Internally the array would be still needed. Also the
> driver would need to somehow "write-back" the value to the offload
> caller and someone (caller/tc) would have to use the array to track
> these bitfields for individual callbacks (probably idr of some sort).
> I don't know, is this excercise worth it?

I was thinking of just doing this on HW stats dump. Drivers which don't
report stats by definition don't need to set any bit :)

> Seems to me like we are overengineering this one a bit.

That's possible, the reporting could be added later... I mostly wanted
to have the discussion.

> Also there would be no "any" it would be type0|type1|type2 the user
> would have to pass. If new type appears, the userspace would have to be
> updated to do "any" again :/ This is inconvenient.

In my proposal above I was suggesting no attr to mean any. I think in
your current code ANY already doesn't include disabled so old user
space should not see any change.