netdev - Re: [RFC PATCH net-next v1 3/3] netdev: add netdev_rx_queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izMzakPfORQ9FX8nh0u0V7awtjUufswCc0Gf3fxxXWX0WA@mail.gmail.com>
Date: Thu, 2 May 2024 09:46:46 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: David Wei <dw@...idwei.uk>
Cc: netdev@...r.kernel.org, Michael Chan <michael.chan@...adcom.com>, 
	Pavan Chebbi <pavan.chebbi@...adcom.com>, 
	Andy Gospodarek <andrew.gospodarek@...adcom.com>, Shailend Chand <shailend@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [RFC PATCH net-next v1 3/3] netdev: add netdev_rx_queue_restart()

On Wed, May 1, 2024 at 6:04 PM David Wei <dw@...idwei.uk> wrote:
>
> On 2024-04-30 11:15 am, Mina Almasry wrote:
> > On Mon, Apr 29, 2024 at 6:07 PM David Wei <dw@...idwei.uk> wrote:
> >>
> >> +err_start_queue:
> >> +       /* Restarting the queue with old_mem should be successful as we haven't
> >> +        * changed any of the queue configuration, and there is not much we can
> >> +        * do to recover from a failure here.
> >> +        *
> >> +        * WARN if the we fail to recover the old rx queue, and at least free
> >> +        * old_mem so we don't also leak that.
> >> +        */
> >> +       if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) {
> >> +               WARN(1,
> >> +                    "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
> >> +                    rxq);
> >> +               dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem);
> >> +       }
> >> +
> >> +err_free_new_mem:
> >> +       dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem);
> >> +       rtnl_unlock();
> >> +
> >> +       return err;
> >> +}
> >> +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart);
> >
> > Does stuff outside of core need this? I don't think so, right? I think
> > you can drop EXPORT_SYMBOL_GPL.
>
> Not sure, we intend to call this from within io_uring. Does that require
> exporting or not?
>

I don't this this requires exporting, no.

> Later on I'll want to add something like
> netdev_rx_queue_set_memory_provider() which then calls
> netdev_rx_queue_restart(). When that happens I can remove the
> EXPORT_SYMBOL_GPL.
>

Sorry, I think if we don't need the EXPORT, then I think don't export
in the first place. Removing an EXPORT is, AFAIU, tricky. Because if
something is exported and then you unexport it could break an out of
tree module/driver that developed a dependency on it. Not sure how
much of a concern it really is.


> >
> > At that point the compiler may complain about an unused function, I
> > think, so maybe  __attribute__((unused)) would help there.
> >
> > I also think it's fine for this patch series to only add the ndos and
> > to leave it to the devmem series to introduce this function, but I'm
> > fine either way.
> >
>
> I'd like to agree on the netdev public API and merge alongside the queue
> api changes, separate to TCP devmem. Then that's one fewer deps between
> our main patchsets.

Ah, OK, sounds good.

I was thinking both efforts would use the ndos, but I think you're
thinking both efforts would use the netdev_rx_queue_restart. Yes, that
sounds reasonable.


--
Thanks,
Mina