On Fri, Nov 1, 2024 at 11:29 PM Mina Almasry <almasrymina@xxxxxxxxxx> wrote:
>
> Hi Taehee, sorry for the late reply. I was out on vacation and needed
> to catch up on some stuff when I got back.
Hi Mina,
Thank you so much for your review :)
>
> On Tue, Oct 22, 2024 at 9:25 AM Taehee Yoo <ap420073@xxxxxxxxx> wrote:
> >
> > If driver doesn't support ring parameter or tcp-data-split configuration
> > is not sufficient, the devmem should not be set up.
> > Before setup the devmem, tcp-data-split should be ON and
> > header-data-split-thresh value should be 0.
> >
> > Tested-by: Stanislav Fomichev <sdf@xxxxxxxxxxx>
> > Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> > ---
> >
> > v4:
> > - Check condition before __netif_get_rx_queue().
> > - Separate condition check.
> > - Add Test tag from Stanislav.
> >
> > v3:
> > - Patch added.
> >
> > net/core/devmem.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/net/core/devmem.c b/net/core/devmem.c
> > index 11b91c12ee11..3425e872e87a 100644
> > --- a/net/core/devmem.c
> > +++ b/net/core/devmem.c
> > @@ -8,6 +8,8 @@
> > */
> >
> > #include <linux/dma-buf.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/ethtool_netlink.h>
> > #include <linux/genalloc.h>
> > #include <linux/mm.h>
> > #include <linux/netdevice.h>
> > @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
> > struct net_devmem_dmabuf_binding *binding,
> > struct netlink_ext_ack *extack)
> > {
> > + struct kernel_ethtool_ringparam kernel_ringparam = {};
> > + struct ethtool_ringparam ringparam = {};
> > struct netdev_rx_queue *rxq;
> > u32 xa_idx;
> > int err;
> > @@ -140,6 +144,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
> > return -ERANGE;
> > }
> >
> > + if (!dev->ethtool_ops->get_ringparam)
> > + return -EOPNOTSUPP;
> > +
>
> Maybe an error code not EOPNOTSUPP. I think that gets returned when
> NET_DEVMEM is not compiled in and other situations like that. Lets
> pick another error code? Maybe ENODEV.
There are several same code in the ethtool core.
It returns EOPNOTSUPP consistently.
In the v3 series, Brett reviewed it.
So, I changed it from EINVAL to EOPNOTSUPP it was reasonable to me.
So I prefer EOPNOTSUPP but I will follow your decision.
What do you think?
>
> Also consider extack error message. But it's very unlikely to hit this
> error, so maybe not necessary.
I removed extack from the v3. because ethtool doesn't use extack for
the same logic. It was reasonable to me.
>
> > + dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam,
> > + extack);
> > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
> > + NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
> > + return -EINVAL;
> > + }
> > + if (kernel_ringparam.hds_thresh) {
> > + NL_SET_ERR_MSG(extack, "header-data-split-thresh is not zero");
> > + return -EINVAL;
> > + }
> > +
>
> Thinking about drivers that support tcp-data-split, but don't support
> any kind of hds_thresh. For us (GVE), the hds_thresh is implicitly
> always 0.
>
> Does the driver need to explicitly set hds_thresh to 0? If so, that
> adds a bit of churn to driver code. Is it possible to detect in this
> function that the driver doesn't support hds_thresh and allow the
> binding if so?
>
> I see in the previous patch you do something like:
>
> supported_ring_params & ETHTOOL_RING_USE_HDS_THRS
>
> To detect there is hds_thresh support. I was wondering if something
> like this is possible so we don't have to update GVE and all future
> drivers to explicitly set thresh to 0.
How about setting maximum hds_threshold to 0?
The default value of hds_threshold of course 0.
I think gve code does not need to change much, just adding like below
will be okay.
I think if drivers don't support setting hds_threshold explicitly, it
is actually the same as support only 0.
So, there is no problem I think.
I didn't analyze gve driver code, So I might think it too optimistically.
#define GVE_HDS_THRESHOLD_MAX 0
kernel_ering->hds_thresh = GVE_HDS_THRESHOLD_MAX;
kernel_ering->hds_thresh_max = GVE_HDS_THRESHOLD_MAX;
...
.supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT |
ETHTOOL_RING_USE_HDS_THRS,
ethtool command may show like this.
ethtool -g enp7s0f3np3
Ring parameters for enp7s0f3np3:
Pre-set maximums:
...
Header data split thresh: 0
Current hardware settings:
...
TCP data split: on
Header data split thresh: 0
If a driver can't set up hds_threshold, ethtool command may show like this.
ethtool -g enp7s0f3np3
Ring parameters for enp7s0f3np3:
Pre-set maximums:
TX push buff len: n/a
Header data split thresh: n/a
Current hardware settings:
...
TCP data split: on
Header data split thresh: n/a
Thanks a lot!
Taehee Yoo
>
> Other than that, looks fine to me.
>
>
> --
> Thanks,
> Mina
