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: <20240419202535.5c5097fe@kernel.org>
Date: Fri, 19 Apr 2024 20:25:35 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
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, 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.