[email protected] writes:
>> Memory-provider queue configuration is validated when the provider is
>> bound. A later ethtool ring change may invalidate it because drivers
>> can size queue memory from both ring depth and RX page size. For
>> example, fbnic uses multiple PPQ entries for each RX page larger than
>> PAGE_SIZE, which reduces the usable software ring depth.
> [ ... ]
>> Use the same transaction for ioctl and netlink. Drivers without
>> ndo_validate_qcfg skip the new validation.
>
> [Severity: Low]
> The message reads as if it closes a live misconfiguration hole, but at this
> commit no in-tree driver implements a ring-depth-aware ndo_validate_qcfg and
> no driver reads qcfg->rx_ring_size, so netdev_queue_config_revalidate()
> cannot change any validation outcome yet.
>
> The two current ndo_validate_qcfg implementations, bnxt_validate_qcfg() and
> mlx5e_queue_validate_qcfg(), only inspect qcfg->rx_page_size. The fbnic
> consumer named in the message, fbnic_validate_qcfg() reading
> qcfg->rx_jumbo_ring_size, arrives in the following patch of the series.
>
> The last paragraph does disclose the limitation, so this is only about
> framing: could the message state that this is core plumbing and that the
> fbnic consumer lands in the next patch?
Ok! I reworded the commit message to state that fbnic consumer followes
the next patch.
...
>> diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
>> index b101341e32513..1975de42a60d9 100644
>> --- a/net/core/netdev_config.c
>> +++ b/net/core/netdev_config.c
>> @@ -50,6 +50,15 @@ static int netdev_nop_validate_qcfg(struct net_device
>> *dev,
>> return 0;
>> }
>>
>> +static void netdev_qcfg_apply_dev(struct netdev_queue_config *qcfg,
>> + const struct netdev_config *cfg)
>> +{
>> + /* Device config overrides callback-provided fallbacks. */
>> + qcfg->rx_ring_size = cfg->rings.rx_pending;
>> + qcfg->rx_mini_ring_size = cfg->rings.rx_mini_pending;
>> + qcfg->rx_jumbo_ring_size = cfg->rings.rx_jumbo_pending;
>> +}
>> +
>
> [Severity: Low]
> Can these three assignments hand a rendered config full of zeroes to
> ndo_validate_qcfg()?
Yes, unless thte driver initializes the fields. Device-wide config is
authoritative -- including zero! I've clarified the requirements in the
docs for next version.
>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> index 4b0bc503f9307..1320289025b25 100644
>> --- a/net/ethtool/ioctl.c
>> +++ b/net/ethtool/ioctl.c
> [ ... ]
>> @@ -2239,10 +2240,29 @@ static int ethtool_set_ringparam(struct net_device
>> *dev, void __user *useraddr)
>> ringparam.tx_pending > max.tx_max_pending)
>> return -EINVAL;
>>
>> + ret = netdev_reconfig_start(dev);
>> + if (ret)
>> + return ret;
>> +
>> + ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> +
>> + ret = netdev_queue_config_revalidate(dev, NULL);
>> + if (ret)
>> + goto out_free_cfg;
>> +
>> ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>> &kernel_ringparam, NULL);
>> - if (!ret)
>> - ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
>> + if (ret)
>> + goto out_free_cfg;
>> +
>> + /* The driver may adjust the accepted ring depths. */
>> + ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> + swap(dev->cfg, dev->cfg_pending);
>> + ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
>
> [Severity: Low]
> Is the committed configuration guaranteed to be one that ndo_validate_qcfg()
> actually saw?
A driver implementing ndo_validate_qcfg must use the same normalization.
I documented that requirement, fbnic uses the same rounding helper in
both paths.
>> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
>> index 9054c89c5d7bb..c04312fc0d061 100644
>> --- a/net/ethtool/rings.c
>> +++ b/net/ethtool/rings.c
> [ ... ]
>> @@ -299,10 +300,20 @@ ethnl_set_rings(struct ethnl_req_info *req_info,
>> struct genl_info *info)
>>
>> dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
>> dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
>> + ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> +
>> + ret = netdev_queue_config_revalidate(dev, info->extack);
>> + if (ret)
>> + return ret;
>>
>> ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>> &kernel_ringparam, info->extack);
>> - return ret < 0 ? ret : 1;
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* The driver may adjust the accepted ring depths. */
>> + ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> + return 1;
>> }
>
> [Severity: Low]
> The comment says the driver may adjust the accepted ring depths, but does
> this second copy store the depths the hardware ended up with, or just the
> requested ones?
It captures only adjustments that the driver reports through ringparam.
I updated the comments and docs.
Björn
