Thu, Oct 12, 2017 at 09:05:10AM CEST, alexander.du...@gmail.com wrote: >On Wed, Oct 11, 2017 at 1:58 PM, Jiri Pirko <j...@resnulli.us> wrote: >> Wed, Oct 11, 2017 at 10:46:52PM CEST, da...@davemloft.net wrote: >>>From: Jiri Pirko <j...@resnulli.us> >>>Date: Wed, 11 Oct 2017 22:38:32 +0200 >>> >>>> Wed, Oct 11, 2017 at 07:46:27PM CEST, alexander.du...@gmail.com wrote: >>>>>On Wed, Oct 11, 2017 at 5:56 AM, Jiri Pirko <j...@resnulli.us> wrote: >>>>>> Wed, Oct 11, 2017 at 02:24:12AM CEST, amritha.namb...@intel.com wrote: >>>>>>>This patch series enables configuring cloud filters in i40e >>>>>>>using the tc-flower classifier. The classification function >>>>>>>of the filter is to match a packet to a class. cls_flower is >>>>>>>extended to offload classid to hardware. The offloaded classid >>>>>>>is used direct matched packets to a traffic class on the device. >>>>>>>The approach here is similar to the tc 'prio' qdisc which uses >>>>>>>the classid for band selection. The ingress qdisc is called ffff:0, >>>>>>>so traffic classes are ffff:1 to ffff:8 (i40e has max of 8 TCs). >>>>>> >>>>>> >>>>>> NACK. This clearly looks like abuse of classid to something >>>>>> else. Classid is here to identify qdisc instance. However, you use it >>>>>> for hw tclass identification. This is mixing of apples and oranges. >>>>>> >>>>>> Why? >>>>>> >>>>>> Please don't try to abuse things! This is not nice. >>>>> >>>>>This isn't an abuse. This is reproducing in hardware what is already >>>>>the behavior for software. Isn't that how offloads are supposed to >>>>>work? >>>> >>>> What is meaning of classid in HW? Classid is SW only identification of >>>> qdisc instances. No relation to HW instances = abuse. >>> >>>Jiri I really don't see what the problem is. >>> >>>As long as the driver does the right thing when changes are made to the >>>qdisc, it doesn't really matter what "key" they use to refer to it. >>> >>>It could have just as easily used the qdisc pointer and then internally >>>use some IDR allocated ID to refer to it in the driver and hardware. >>> >>>But that's such a waste, we have a unique handle already so why can't >>>the driver just use that? >> >> Well if I see classid, I expect it should refer to qdisc instance. So >> far, this has been always a case. But for some drivers, this would mean >> something totally different and unrelated. So what should I think? >> What's next? Classid could be abused to identify something else. I don't >> understand why. > >The general idea is we are trying to offload some of the qdisc work >down into the hardware. It is kind of hard to do that without >providing this sort of abstraction.
Well you expect classid being 0-7, to identify the tclass in hw. What is I pass something else? I think that what DaveM suggests makes sense. You should accept every classid and map it to 0-7 internally in driver. > >> classid in kernel and tclass in hw are 2 completely unrelated things. >> Why they should share the same userspace api? What am I missing that >> indicates this is not an abuse? > >This is both true and not quite true. In the case of mqprio it is >already creating virtual qdiscs for each traffic class. That is >essentially what we are trying to emulate on the receive side. That >was why we thought we might use this abstraction. > >> There should be clean and well-defined userspace api: >> 1) classid to identify qdisc instances >> 2) something else to identify HW tclasses > >I agree with the well defined userspace api portion of this. However I >somewhat disagree on the HW tclasses argument as we have virtual >qdiscs floating around inside of mqprio for instance that represent >the same type of thing. You will find that the classid values with a >minor value less than or equal to the number of TCs don't actually >exist other than for collecting statistics. If that is all you are >looking for we could probably update ingress and clsact to at a >minimum display the class IDs and treat them as full virtual classids >within the qdisc. I figure it wouldn't make sense to add statistics Agreed. I'm not happy how clsact ingress/egress classids are implemented. The virtual qdiscs you are suggesting make sense. >since they don't actually enqueue any packets. > >One thought I am considering, is what if we change the class ID of the >virtual qdiscs for mqprio that represent priority based traffic >classes so that we reserved TC_H_MIN values 0xFFE0 - 0xFFEF to >represent traffic classes 0 through 15? The advantage would be that it >would make the classid layout for mqprio closer to what is already >there for mq, and then in addition we would have a block of values we >could use as reserved for mq, mqprio, ingress, and clsact to represent >what you refer to as the HW tclasses since mqprio is already doing >something like this. > >- Alex