netdev - Re: [RFC PATCH net-next v1 1/3] 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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <15333409-0cad-4580-b093-aeae58664034@davidwei.uk>
Date: Wed, 1 May 2024 19:44:36 -0700
From: David Wei <dw@...idwei.uk>
To: Mina Almasry <almasrymina@...gle.com>
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 1/3] queue_api: define queue api

On 2024-04-30 11:00 am, Mina Almasry wrote:
> On Mon, Apr 29, 2024 at 6:07 PM David Wei <dw@...idwei.uk> wrote:
>>
>> From: Mina Almasry <almasrymina@...gle.com>
>>
>> This API enables the net stack to reset the queues used for devmem TCP.
>>
>> Signed-off-by: Mina Almasry <almasrymina@...gle.com>
>> Signed-off-by: David Wei <dw@...idwei.uk>
>> ---
>>  include/linux/netdevice.h   |  3 +++
>>  include/net/netdev_queues.h | 27 +++++++++++++++++++++++++++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index f849e7d110ed..6a58ec73c5e8 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1957,6 +1957,7 @@ enum netdev_reg_state {
>>   *     @sysfs_rx_queue_group:  Space for optional per-rx queue attributes
>>   *     @rtnl_link_ops: Rtnl_link_ops
>>   *     @stat_ops:      Optional ops for queue-aware statistics
>> + *     @queue_mgmt_ops:        Optional ops for queue management
>>   *
>>   *     @gso_max_size:  Maximum size of generic segmentation offload
>>   *     @tso_max_size:  Device (as in HW) limit on the max TSO request size
>> @@ -2340,6 +2341,8 @@ struct net_device {
>>
>>         const struct netdev_stat_ops *stat_ops;
>>
>> +       const struct netdev_queue_mgmt_ops *queue_mgmt_ops;
>> +
>>         /* for setting kernel sock attribute on TCP connection setup */
>>  #define GSO_MAX_SEGS           65535u
>>  #define GSO_LEGACY_MAX_SIZE    65536u
>> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
>> index 1ec408585373..337df0860ae6 100644
>> --- a/include/net/netdev_queues.h
>> +++ b/include/net/netdev_queues.h
>> @@ -60,6 +60,33 @@ struct netdev_stat_ops {
>>                                struct netdev_queue_stats_tx *tx);
>>  };
>>
>> +/**
>> + * struct netdev_queue_mgmt_ops - netdev ops for queue management
>> + *
>> + * @ndo_queue_mem_alloc: Allocate memory for an RX queue. The memory returned
>> + *                      in the form of a void* can be passed to
>> + *                      ndo_queue_mem_free() for freeing or to ndo_queue_start
>> + *                      to create an RX queue with this memory.
>> + *
>> + * @ndo_queue_mem_free:        Free memory from an RX queue.
>> + *
>> + * @ndo_queue_start:   Start an RX queue at the specified index.
>> + *
>> + * @ndo_queue_stop:    Stop the RX queue at the specified index.
>> + */
>> +struct netdev_queue_mgmt_ops {
>> +       void *                  (*ndo_queue_mem_alloc)(struct net_device *dev,
>> +                                                      int idx);
>> +       void                    (*ndo_queue_mem_free)(struct net_device *dev,
>> +                                                     void *queue_mem);
>> +       int                     (*ndo_queue_start)(struct net_device *dev,
>> +                                                  int idx,
>> +                                                  void *queue_mem);
>> +       int                     (*ndo_queue_stop)(struct net_device *dev,
>> +                                                 int idx,
>> +                                                 void **out_queue_mem);
>> +};
> 
> Sorry, I think we raced a bit, we updated our interface definition
> based on your/Jakub's feedback to expose the size of the struct for
> core to allocate, so it looks like this for us now:
> 
> +struct netdev_queue_mgmt_ops {
> +       size_t                  ndo_queue_mem_size;
> +       int                     (*ndo_queue_mem_alloc)(struct net_device *dev,
> +                                                      void *per_queue_mem,
> +                                                      int idx);
> +       void                    (*ndo_queue_mem_free)(struct net_device *dev,
> +                                                     void *per_queue_mem);
> +       int                     (*ndo_queue_start)(struct net_device *dev,
> +                                                  void *per_queue_mem,
> +                                                  int idx);
> +       int                     (*ndo_queue_stop)(struct net_device *dev,
> +                                                 void *per_queue_mem,
> +                                                 int idx);
> +};
> +
> 
> The idea is that ndo_queue_mem_size is the size of the memory
> per_queue_mem points to.
> 
> The rest of the functions are slightly modified to allow core to
> allocate the memory and pass in the pointer for the driver to fill
> in/us. I think Shailend is close to posting the patches, let us know
> if you see any issues.
> 

Hmm. Thinking about this a bit more, are you having netdev core manage
all of the queues, i.e. alloc/free during open()/stop()? Otherwise how
can netdev core pass in the old queue mem into ndo_queue_stop(), and
where is the queue mem stored?

Or is it the new queue mem being passed into ndo_queue_stop()?

My takeaway from the discussion on Shailend's last RFC is that for the
first iteration we'll keep what we had before and have the driver
alloc/free the qmem. Implementing this for bnxt felt pretty good as
well.