On 2021/8/21 6:21, Jakub Kicinski wrote:
> On Fri, 20 Aug 2021 21:27:13 +0300 Grygorii Strashko wrote:
>> This is very big change which is not only mix two separate changes, but also looks
>> half-done. From one side you're adding HW feature supported by limited number of HW,
>> from another - changing most of net drivers to support it by generating mix of legacy
>> and new kernel_ethtool_coalesce parameters.
>>
>> There is also an issue - you do not account get/set_per_queue_coalesce() in any way.
>
> ethtool's netlink interface does not support per queue coalescing.
> I don't think it's fair to require it as part of this series.
>
>> Would it be possible to consider following, please?
>>
>> - move extack change out of this series
>
> Why? To have to change all the drivers twice?
>
>> - option (a)
>> add new callbacks in ethtool_ops as set_coalesce_cqe/get_coalesce_cqe for CQE support.
>> Only required drivers will need to be changed.
>
> All the params are changed as one operation from user space's
> perspective. Having two ops would make it problematic for drivers
> to fail the entire op without modifying half of the parameters in
> a previous callback.
>
>> - option (b)
>> add struct ethtool_coalesce as first field of kernel_ethtool_coalesce
>
> This ends up being more painful than passing an extra parameter
> in my experience.
>
>> struct kernel_ethtool_coalesce {
>> /* legacy */
>> struct ethtool_coalesce coal;
>>
>> /* new */
>> u8 use_cqe_mode_tx;
>> u8 use_cqe_mode_rx;
>> };
>>
>> -- then b.1
>> drivers can be updated as
>> static int set_coalesce(struct net_device *dev,
>> struct kernel_ethtool_coalesce *kernel_coal)
>> {
>> struct ethtool_coalesce *coal = &kernel_coal->coal;
>>
>> (which will clearly indicate migration to the new interface )
>>
>> -- then b.2
>> add new callbacks in ethtool_ops as set_coalesce_ext/get_coalesce_ext (extended)
>> which will accept struct kernel_ethtool_coalesce as parameter an allow drivers to migrate when needed
>> (or as separate patch which will do only migration).
>>
>> Personally, I like "b.2".
>
> These options were considered. None of the options is great to
> be honest. Let's try the new params, I say.
> .
>
Yes, these have been considered in previous RFCs. For details, please refer to [1].
[1]https://lore.kernel.org/netdev/20210526165633.3f7982c9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
