Date:   Tue, 3 Mar 2020 14:20:35 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
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

Mon, Mar 02, 2020 at 08:39:33PM CET, kuba@...nel.org wrote:
>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.

Okay.

>
>> 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.

Odd, no attribute meaning "any". I think it is polite to fillup the
attribute for dump if kernel supports the attribute. However, here, we
would not fill it up in case of "any". That is quite odd.

We can have a bit that would mean "any" though. What do you think?

Powered by blists - more mailing lists