Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

On Tue, 2016-05-10 at 11:24 +0000, David Laight wrote:
> From: Ben Hutchings
> > 
> > Sent: 09 May 2016 01:17
> > On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> > > 
> > > 
> > > On Sat, 7 May 2016, Ben Hutchings wrote:
> > > 
> > > > 
> > > > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > > > [...]
> > > > > 
> > > > > +static int
> > > > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > > > +{
> > > > [...]
> > > > > 
> > > > > +   switch (ec->rx_coalesce_usecs) {
> > > > > +   case VMXNET3_COALESCE_DEFAULT:
> > > > > +   case VMXNET3_COALESCE_DISABLED:
> > > > > +   case VMXNET3_COALESCE_ADAPT:
> > > > > +           if (ec->tx_max_coalesced_frames ||
> > > > > +               ec->tx_max_coalesced_frames_irq ||
> > > > > +               ec->rx_max_coalesced_frames_irq) {
> > > > > +                   return -EINVAL;
> > > > > +           }
> > > > > +           memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > > > +           adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > > > +           break;
> > > > > +   case VMXNET3_COALESCE_STATIC:
> > > > [...]
> > > > 
> > > > I don't want to see drivers introducing magic values for fields that
> > > > are denominated in microseconds (especially not for 0, which is the
> > > > correct way to specify 'no coalescing').  If the current
> > > > ethtool_coalesce structure is inadequate, propose an extension.
> > > For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to
> > > the device.
> > > 
> > > Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> > > 
> > > rx-usecs = 0 -> coalescing disabled.
> > > rx-usecs = 1 -> default (chosen by the device).
> > > rx-usecs = 2 -> adaptive coalescing.
> > > rx-usecs = 3 -> static coalescing.
> Would it be better to use stupidly large values for the non-zero special values?
> That would less problematic if someone expects 2 to mean '2 usecs'.

That seems less problematic, but I think we'd have to check that
current drivers (or at least the most widely used ones) reject those
values.

We should still need to standardise names and definitions for those
special values, so that the ethtool utility can provide keywords and
documentation for them.

Ben.

-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]