Re: [Intel-wired-lan] [PATCH iwl-next v4 0/5] iavf: Add devlink and devlink rate support'

["Zhang, Xuejun" <xuejun.zhang@intel.com>]

On 11/18/2023 8:48 AM, Jakub Kicinski wrote:
> On Thu, 16 Nov 2023 21:52:49 -0800 Zhang, Xuejun wrote:
>> Thanks for looking into our last patch with devlink API. Really
>> appreciate your candid review.
>>
>> Following your suggestion, we have looked into 3 tc offload options to
>> support queue rate limiting
>>
>> #1 mq + matchall + police
>>
>> #2 mq + tbf
> You can extend mqprio, too, if you wanted.
>
>> #3 htb
>>
>> all 3 tc offload options require some level of tc extensions to support
>> VF tx queue rate limiting (tx_maxrate & tx_minrate)
>>
>> htb offload requires minimal tc changes or no change with similar change
>> done @ driver (we can share patch for review).
>>
>> After discussing with Maxim Mikityanskiy(
>> https://lore.kernel.org/netdev/54a7dd27-a612-46f1-80dd-b43e28f8e4ce@intel.com/
>> ), looks like sysfs interface with tx_minrate extension could be the
>> option we can take.
>>
>> Look forward your opinion & guidance. Thanks for your time!
> My least favorite thing to do is to configure the same piece of silicon
> with 4 different SW interfaces. It's okay if we have 4 different uAPIs
> (user level APIs) but the driver should not be exposed to all these
> options.
>
> I'm saying 4 but really I can think of 6 ways of setting maxrate :(
>
> IMHO we need to be a bit more realistic about the notion of "offloading
> the SW thing" for qdiscs specifically. Normally we offload SW constructs
> to have a fallback and have a clear definition of functionality.
> I bet most data-centers will use BPF+FQ these days, so the "fallback"
> argument does not apply. And the "clear definition" when it comes to
> basic rate limiting is.. moot.
>
> Besides we already have mqprio, sysfs maxrate, sriov ndo, devlink rate,
> none of which have SW fallback.
>
> So since you asked for my opinion - my opinion is that step 1 is to
> create a common representation of what we already have and feed it
> to the drivers via a single interface. I could just be taking sysfs
> maxrate and feeding it to the driver via the devlink rate interface.
> If we have the right internals I give 0 cares about what uAPI you pick.

There is an existing ndo api for setting tx_maxrate API

int (*ndo_set_tx_maxrate)(struct net_device *dev,
                           int queue_index,
               u32 maxrate);

we could expand and modify the above API with tx_minrate and burst 
parameters as below
int (*ndo_set_tx_rate)(struct net_device *dev,
                  int queue_index,
                int maxrate , int minrate, int burst);

queue_index: tx queue index of net device
maxrate: tx maxrate in mbps
minrate: tx min rate in mbps
burst: data burst in bytes


The proposed API would incur net/core and driver changes as follows
a) existing drivers with ndo_set_tx_maxrate support upgraded to use new 
ndo_set_tx_rate
b) net sysfs (replacing ndo_set_maxrate with ndo_set_tx_rate with 
minrate and burst set to 0, -1 means ignore)
c) Keep the existing /sys/class/net/ethx/queues/tx_nn/tx_maxrate as it 
is currently
d) Add sysfs entry as /sys/class/net/ethx/queues/tx_nn/tx_minrate & 
/sys/class/net/ethx/queues/tx_nn/burst

Let us know your thoughts.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan