Message-ID: <CAHS8izO=Vc6Kxx620_y6v-3PtRL3_UFP6zDRfgLf85SXpP0+dQ@mail.gmail.com>
Date: Fri, 19 Apr 2024 09:10:42 -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 Thu, Apr 18, 2024 at 6:48 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 18 Apr 2024 19:51:59 +0000 Shailend Chand wrote:
> > +static int gve_rx_queue_stop(struct net_device *dev, int idx,
> > +                          void **out_per_q_mem)
> > +{
> > +     struct gve_priv *priv = netdev_priv(dev);
> > +     struct gve_rx_ring *rx;
> > +     int err;
> > +
> > +     if (!priv->rx)
> > +             return -EAGAIN;
> > +     if (idx < 0 || idx >= priv->rx_cfg.max_queues)
> > +             return -ERANGE;
>
> A little too defensive? Core should not issue these > current real num
> queues.
>
> > +     /* Destroying queue 0 while other queues exist is not supported in DQO */
> > +     if (!gve_is_gqi(priv) && idx == 0)
> > +             return -ERANGE;
> > +
> > +     rx = kvzalloc(sizeof(*rx), GFP_KERNEL);
>
> Why allocate in the driver rather than let the core allocate based on
> the declared size ?
>

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);
+}
+
 static int gve_rx_queue_stop(struct net_device *dev, int idx,
-      void **out_per_q_mem)
+      void *out_per_q_mem)
 {
  struct gve_priv *priv = netdev_priv(dev);
- struct gve_rx_ring *rx;
+ struct gve_rx_ring *rx = out_per_q_mem;
  int err;

  if (!priv->rx)
@@ -2595,9 +2600,6 @@ static int gve_rx_queue_stop(struct net_device
*dev, int idx,
  if (!gve_is_gqi(priv) && idx == 0)
  return -ERANGE;

- rx = kvzalloc(sizeof(*rx), GFP_KERNEL);
- if (!rx)
- return -ENOMEM;
  *rx = priv->rx[idx];

  /* Single-queue destruction requires quiescence on all queues */
@@ -2606,7 +2608,6 @@ static int gve_rx_queue_stop(struct net_device
*dev, int idx,
  /* This failure will trigger a reset - no need to clean up */
  err = gve_adminq_destroy_single_rx_queue(priv, idx);
  if (err) {
- kvfree(rx);
  return err;
  }

@@ -2618,7 +2619,6 @@ static int gve_rx_queue_stop(struct net_device
*dev, int idx,
  /* Turn the unstopped queues back up */
  gve_turnup_and_check_status(priv);

- *out_per_q_mem = rx;
  return 0;
 }

@@ -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,
 };

 static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index 337df0860ae6..0af08414d8eb 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h
@@ -75,6 +75,7 @@ struct netdev_stat_ops {
  * @ndo_queue_stop: Stop the RX queue at the specified index.
  */
 struct netdev_queue_mgmt_ops {
+ size_t (*ndo_queue_mem_get_size)(void);
  void * (*ndo_queue_mem_alloc)(struct net_device *dev,
         int idx);
  void (*ndo_queue_mem_free)(struct net_device *dev,
@@ -84,7 +85,7 @@ struct netdev_queue_mgmt_ops {
     void *queue_mem);
  int (*ndo_queue_stop)(struct net_device *dev,
    int idx,
-   void **out_queue_mem);
+   void *out_queue_mem);
 };

 /**
diff --git a/net/core/devmem.c b/net/core/devmem.c
index 01337de7d6a4..89c90e21f083 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -60,7 +60,8 @@ static int net_devmem_restart_rx_queue(struct
net_device *dev, int rxq_idx)
  void *old_mem;
  int err;

- if (!dev->queue_mgmt_ops->ndo_queue_stop ||
+ if (!dev->queue_mgmt_ops->ndo_queue_mem_get_size ||
+     !dev->queue_mgmt_ops->ndo_queue_stop ||
      !dev->queue_mgmt_ops->ndo_queue_mem_free ||
      !dev->queue_mgmt_ops->ndo_queue_mem_alloc ||
      !dev->queue_mgmt_ops->ndo_queue_start)
@@ -70,7 +71,11 @@ static int net_devmem_restart_rx_queue(struct
net_device *dev, int rxq_idx)
  if (!new_mem)
  return -ENOMEM;

- err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
+ old_mem = kvzalloc(dev->queue_mgmt_ops->ndo_queue_mem_get_size(),
+    GFP_KERNEL);
+ BUG_ON(!old_mem); /* TODO */
+
+ err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq_idx, old_mem);
  if (err)
  goto err_free_new_mem;

@@ -79,6 +84,7 @@ static int net_devmem_restart_rx_queue(struct
net_device *dev, int rxq_idx)
  goto err_start_queue;

  dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem);
+ kvfree(old_mem);

  return 0;


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?

--
Thanks,
Mina

Powered by blists - more mailing lists