On 2021/8/23 11:06, moyufeng wrote:
>
>
> 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/
>
> .
>
Hi Grygorii & Jakub
Is this patch still good? Or do I need to resend this series
with RFC link above in change log?
Thanks
