On Thu, Dec 19, 2024 at 11:35 AM Jakub Kicinski <kuba@xxxxxxxxxx> 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
