netdev - Re: [PATCH net-next v6 4/9] net: ethtool: add support for configuring hds-thresh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMArcTUCy2Wkrw5foPheHq=1x5SeDcwp0uvSniGzOGZudVqY+w@mail.gmail.com>
Date: Thu, 19 Dec 2024 23:10:39 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com, 
	almasrymina@...gle.com, donald.hunter@...il.com, corbet@....net, 
	michael.chan@...adcom.com, andrew+netdev@...n.ch, hawk@...nel.org, 
	ilias.apalodimas@...aro.org, ast@...nel.org, daniel@...earbox.net, 
	john.fastabend@...il.com, dw@...idwei.uk, sdf@...ichev.me, 
	asml.silence@...il.com, brett.creeley@....com, linux-doc@...r.kernel.org, 
	netdev@...r.kernel.org, kory.maincent@...tlin.com, 
	maxime.chevallier@...tlin.com, danieller@...dia.com, hengqi@...ux.alibaba.com, 
	ecree.xilinx@...il.com, przemyslaw.kitszel@...el.com, hkallweit1@...il.com, 
	ahmed.zaki@...el.com, rrameshbabu@...dia.com, idosch@...dia.com, 
	jiri@...nulli.us, bigeasy@...utronix.de, lorenzo@...nel.org, 
	jdamato@...tly.com, aleksander.lobakin@...el.com, kaiyuanz@...gle.com, 
	willemb@...gle.com, daniel.zahka@...il.com
Subject: Re: [PATCH net-next v6 4/9] net: ethtool: add support for configuring hds-thresh

On Thu, Dec 19, 2024 at 11:35 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 18 Dec 2024 14:45:25 +0000 Taehee Yoo wrote:
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 4e451084d58a..4f407ce9eed1 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -78,6 +78,8 @@ enum {
> >   * @cqe_size: Size of TX/RX completion queue event
> >   * @tx_push_buf_len: Size of TX push buffer
> >   * @tx_push_buf_max_len: Maximum allowed size of TX push buffer
> > + * @hds_thresh: Threshold value of header-data-split-thresh
> > + * @hds_thresh_max: Maximum allowed threshold of header-data-split-thresh
>
> nit: s/allowed/supported/

Thanks, I will change it.

>
> > +u8 dev_xdp_sb_prog_count(struct net_device *dev)
> > +{
> > +     u8 count = 0;
> > +     int i;
> > +
> > +     for (i = 0; i < __MAX_XDP_MODE; i++)
> > +             if (dev->xdp_state[i].prog &&
> > +                 !dev->xdp_state[i].prog->aux->xdp_has_frags)
> > +                     count++;
> > +     return count;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_xdp_sb_prog_count);
>
> No need to export this, AFAICT, none of the callers can be built
> as a module.

Okay, I will not export this function.

>
> > +     hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;
>
> Does it really matter if we modified the HDS setting for the XDP check?
> Whether it was already set or the current config is asking for it to be
> set having XDP SB and HDS is invalid, we can return an error.

Right, it doesn't need to check modification.
I will remove it.

>
> > +     if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> > +         hds_config_mod && dev_xdp_sb_prog_count(dev)) {
> > +             NL_SET_ERR_MSG(info->extack,
>
>                 NL_SET_ERR_MSG_ATTR(info->extack,

Thanks for it too, I will use it.

>                                     tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT],
>                                     ...
>
> > +                            "tcp-data-split can not be enabled with single buffer XDP");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max) {
> > +             NL_SET_BAD_ATTR(info->extack,
> > +                             tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX]);
> > +             return -ERANGE;
> > +     }
>
> Can this condition not be handled by the big if "ladder" below?
> I mean like this:

Thanks for that, I will try to apply it!

>
> @@ -282,6 +276,8 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
>                 err_attr = tb[ETHTOOL_A_RINGS_RX_JUMBO];
>         else if (ringparam.tx_pending > ringparam.tx_max_pending)
>                 err_attr = tb[ETHTOOL_A_RINGS_TX];
> +       else if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max)
> +               err_attr = tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX];
>         else
>                 err_attr = NULL;
>         if (err_attr) {
>
> >       /* ensure new ring parameters are within limits */
> >       if (ringparam.rx_pending > ringparam.rx_max_pending)
> >               err_attr = tb[ETHTOOL_A_RINGS_RX];

Thanks a lot!
Taehee Yoo