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.
