linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Kubecek <mkubecek@suse.cz>
To: tanhuazhong <tanhuazhong@huawei.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, linuxarm@huawei.com,
	kuba@kernel.org
Subject: Re: [RFC net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
Date: Thu, 19 Nov 2020 23:02:03 +0100	[thread overview]
Message-ID: <20201119220203.fv2uluoeekyoyxrv@lion.mk-sys.cz> (raw)
In-Reply-To: <e43890d1-5596-3439-f4a7-d704c069a035@huawei.com>

On Thu, Nov 19, 2020 at 04:56:42PM +0800, tanhuazhong wrote:
> On 2020/11/19 12:15, Andrew Lunn wrote:
> > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > > index 9ca87bc..afd8de2 100644
> > > --- a/include/uapi/linux/ethtool.h
> > > +++ b/include/uapi/linux/ethtool.h
> > > @@ -433,6 +433,7 @@ struct ethtool_modinfo {
> > >    *	a TX interrupt, when the packet rate is above @pkt_rate_high.
> > >    * @rate_sample_interval: How often to do adaptive coalescing packet rate
> > >    *	sampling, measured in seconds.  Must not be zero.
> > > + * @use_dim: Use DIM for IRQ coalescing, if adaptive coalescing is enabled.
> > >    *
> > >    * Each pair of (usecs, max_frames) fields specifies that interrupts
> > >    * should be coalesced until
> > > @@ -483,6 +484,7 @@ struct ethtool_coalesce {
> > >   	__u32	tx_coalesce_usecs_high;
> > >   	__u32	tx_max_coalesced_frames_high;
> > >   	__u32	rate_sample_interval;
> > > +	__u32	use_dim;
> > >   };
> > 
> > You cannot do this.
> > 
> > static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
> >                                                     void __user *useraddr)
> > {
> >          struct ethtool_coalesce coalesce;
> >          int ret;
> > 
> >          if (!dev->ethtool_ops->set_coalesce)
> >                  return -EOPNOTSUPP;
> > 
> >          if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
> >                  return -EFAULT;
> > 
> > An old ethtool binary is not going to set this extra last byte to
> > anything meaningful. You cannot tell if you have an old or new user
> > space, so you have no idea if it put anything into use_dim, or if it
> > is random junk.

Even worse, as there is no indication of data length, ETHTOOL_GCOALESCE
ioctl request from old ethtool on new kernel would result in kernel
writing past the end of userspace buffer.

> > You have to leave the IOCTL interface unchanged, and limit this new
> > feature to the netlink API.
> > 
> 
> Hi, Andrew.
> thanks for pointing out this problem, i will fix it.
> without callling set_coalesce/set_coalesce of ethtool_ops, do you have any
> suggestion for writing/reading this new attribute to/from the driver? add a
> new field in net_device or a new callback function in ethtool_ops seems not
> good.

We could use a similar approach as struct ethtool_link_ksettings, e.g.

	struct kernel_ethtool_coalesce {
		struct ethtool_coalesce base;
		/* new members which are not part of UAPI */
	}

get_coalesce() and set_coalesce() would get pointer to struct
kernel_ethtool_coalesce and ioctl code would be modified to only touch
the base (legacy?) part.

While already changing the ops arguments, we could also add extack
pointer, either as a separate argument or as struct member (I slightly
prefer the former).

BtW, please don't forget to update the message descriptions in
Documentation/networking/ethtool-netlink.rst

Michal

  reply	other threads:[~2020-11-19 22:02 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19  3:54 [RFC net-next 0/2] net: updates for -next Huazhong Tan
2020-11-19  3:54 ` [RFC net-next 1/2] ethtool: add support for controling the type of adaptive coalescing Huazhong Tan
2020-11-19  4:15   ` Andrew Lunn
2020-11-19  8:56     ` tanhuazhong
2020-11-19 22:02       ` Michal Kubecek [this message]
2020-11-20  1:52         ` tanhuazhong
2020-11-20  2:59         ` tanhuazhong
2020-11-20  7:23           ` Michal Kubecek
2020-11-20 13:39             ` Andrew Lunn
2020-11-20 21:22               ` Michal Kubecek
2020-11-21  1:56                 ` tanhuazhong
2020-11-19 21:43     ` Michal Kubecek
2020-11-19  3:54 ` [RFC net-next 2/2] net: hns3: add support for dynamic interrupt moderation Huazhong Tan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201119220203.fv2uluoeekyoyxrv@lion.mk-sys.cz \
    --to=mkubecek@suse.cz \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=netdev@vger.kernel.org \
    --cc=tanhuazhong@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).