On Fri, Nov 1, 2024 at 11:03 AM Taehee Yoo <ap420073@xxxxxxxxx> wrote:
>
> 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.
>
Ah, looks like I accidentally re-opened discussion on a couple of
items that you're already aligned on. Not critical. This is fine by
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,
>
OK, if you can think of a way to do this without any churn to other
drivers, that would be better, but this is fine by me either way.
Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>
--
Thanks,
Mina
