Message-ID: <CAHS8izN0_v0UTNT9Xu8yJcvStvAYBQW3kadMG_0d4xr7Ta-VVQ@mail.gmail.com>
Date: Fri, 1 Nov 2024 13:40:51 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Taehee Yoo <ap420073@...il.com>, Harshitha Ramamurthy <hramamurthy@...gle.com>, 
	Ziwei Xiao <ziweixiao@...gle.com>
Cc: Praveen Kaligineedi <pkaligineedi@...gle.com>, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, edumazet@...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 v4 5/8] net: devmem: add ring parameter filtering

On Fri, Nov 1, 2024 at 11:03 AM Taehee Yoo <ap420073@...il.com> wrote:
>
> On Fri, Nov 1, 2024 at 11:29 PM Mina Almasry <almasrymina@...gle.com> 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@...il.com> 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@...ichev.me>
> > > Signed-off-by: Taehee Yoo <ap420073@...il.com>
> > > ---
> > >
> > > 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@...gle.com>

-- 
Thanks,
Mina

Powered by blists - more mailing lists