From: tanhuazhong <tanhuazhong@huawei.com>
To: Michal Kubecek <mkubecek@suse.cz>
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: Fri, 20 Nov 2020 09:52:10 +0800 [thread overview]
Message-ID: <a4805c93-300b-1599-359a-1ed94a5eb77d@huawei.com> (raw)
In-Reply-To: <20201119220203.fv2uluoeekyoyxrv@lion.mk-sys.cz>
On 2020/11/20 6:02, Michal Kubecek wrote:
> 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.
>
Thanks for your suggestion. i will implement it in V2.
> 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
>
will update the doc in V2.
Thanks.
Huazhong.
> .
>
next prev parent reply other threads:[~2020-11-20 1:52 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
2020-11-20 1:52 ` tanhuazhong [this message]
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=a4805c93-300b-1599-359a-1ed94a5eb77d@huawei.com \
--to=tanhuazhong@huawei.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
/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).