Mon, Mar 02, 2020 at 08:39:33PM CET, kuba@kernel.org wrote:

On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote:

quoted

quoted

quoted

quoted

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.
  

quoted

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

quoted

quoted

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.
 

quoted

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 :)

quoted

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.

quoted

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?