netdev - Re: [RFC PATCH net-next 9/9] gve: Implement queue api

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHS8izOEbZ6wdw2=pPt_P1F81qQxjw83foeQ9baZk0XwYEmmpg@mail.gmail.com>
Date: Mon, 22 Apr 2024 09:58:38 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Shailend Chand <shailend@...gle.com>, netdev@...r.kernel.org, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com, willemb@...gle.com
Subject: Re: [RFC PATCH net-next 9/9] gve: Implement queue api

On Fri, Apr 19, 2024 at 8:25 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 19 Apr 2024 09:10:42 -0700 Mina Almasry wrote:
> > Currently the ndos don't include an interface for the driver to
> > declare the size, right? In theory we could add it to the ndos like
> > so, if I understood you correctly (untested yet, just to illustrate
> > what I'm thinking point):
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c
> > b/drivers/net/ethernet/google/gve/gve_main.c
> > index 7c38dc06a392..efe3944b529a 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -2579,11 +2579,16 @@ static void gve_write_version(u8 __iomem
> > *driver_version_register)
> >   writeb('\n', driver_version_register);
> >  }
> >
> > +static size_t gve_rx_queue_mem_get_size(void)
> > +{
> > + return sizeof(struct gve_rx_ring);
> > +}
>
> > @@ -2709,6 +2709,7 @@ static const struct netdev_queue_mgmt_ops
> > gve_queue_mgmt_ops = {
> >   .ndo_queue_mem_free = gve_rx_queue_mem_free,
> >   .ndo_queue_start = gve_rx_queue_start,
> >   .ndo_queue_stop = gve_rx_queue_stop,
> > + .ndo_queue_mem_get_size = gve_rx_queue_mem_get_size,
> >  };
>
> I don't think we need to make it a callback, even, directly:
>
> const struct netdev_queue_mgmt_ops gve_queue_mgmt_ops = {
> +       .queue_mem_size         = sizeof(struct gve_rx_ring),
>         .ndo_queue_mem_free     = gve_rx_queue_mem_free,
>         .ndo_queue_start        = gve_rx_queue_start,
>         .ndo_queue_stop         = gve_rx_queue_stop,
>
> > I think maybe if we want to apply this change to mem_stop, then we
> > should probably also apply this change to queue_mem_alloc as well,
> > right? I.e. core will allocate the pointer, and ndo_queue_mem_alloc
> > would allocate the actual resources and would fill in the entries of
> > the pointer? Is this what you're looking for here?
>
> Yup. But thinking about it again, this may be more natural once we also
> have the open/close path use the queue API. IIUC for now the driver
> allocates the queue resources on open, without going via .ndo_queue_*
> If that's the case we can keep the code as you have it here.

Yes, as currently written the queue API funcs & gve_open/close use the
same internal gve helpers, but they do not go via .ndo_queue_*.

OK, sounds like you would like us to keep this bit of the code as we
have it here. Let us know if you have any other feedback. I think
we're working through final testing/polishing/code reviewing
internally and will be submitting something very similar to this as
non-RFC for review, with whatever feedback we receive in the meantime.

-- 
Thanks,
Mina