From: Eric Dumazet <edumazet@google.com>
Sent: Wednesday, May 15, 2024 7:45 AM
To: Dan Jurgens <redacted>
Cc: netdev@vger.kernel.org; mst@redhat.com; jasowang@redhat.com;
xuanzhuo@linux.alibaba.com; virtualization@lists.linux.dev;
davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
[off-list ref]
Subject: Re: [PATCH net-next v6 2/6] virtio_net: Remove command data
from control_buf

On Fri, May 3, 2024 at 10:25 PM Daniel Jurgens [off-list ref] wrote:

quoted

Allocate memory for the data when it's used. Ideally the struct could
be on the stack, but we can't DMA stack memory. With this change only
the header and status memory are shared between commands, which will
allow using a tighter lock than RTNL.

Signed-off-by: Daniel Jurgens <redacted>
Reviewed-by: Jiri Pirko <redacted>
---
 drivers/net/virtio_net.c | 124
+++++++++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 39 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index

9cf93a8a4446..451879d570a8 100644

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -368,15 +368,6 @@ struct virtio_net_ctrl_rss {  struct control_buf

{
        struct virtio_net_ctrl_hdr hdr;
        virtio_net_ctrl_ack status;
-       struct virtio_net_ctrl_mq mq;
-       u8 promisc;
-       u8 allmulti;
-       __virtio16 vid;
-       __virtio64 offloads;
-       struct virtio_net_ctrl_coal_tx coal_tx;
-       struct virtio_net_ctrl_coal_rx coal_rx;
-       struct virtio_net_ctrl_coal_vq coal_vq;
-       struct virtio_net_stats_capabilities stats_cap;
 };

 struct virtnet_info {

@@ -2828,14 +2819,19 @@ static void virtnet_ack_link_announce(struct

virtnet_info *vi)

 static int _virtnet_set_queues(struct virtnet_info *vi, u16
queue_pairs)  {
+       struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
        struct scatterlist sg;
        struct net_device *dev = vi->dev;

        if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
                return 0;

-       vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev,

queue_pairs);

quoted

-       sg_init_one(&sg, &vi->ctrl->mq, sizeof(vi->ctrl->mq));
+       mq = kzalloc(sizeof(*mq), GFP_KERNEL);
+       if (!mq)
+               return -ENOMEM;
+
+       mq->virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
+       sg_init_one(&sg, mq, sizeof(*mq));

        if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
                                  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
&sg)) { @@ -2864,6 +2860,7 @@ static int virtnet_set_queues(struct
virtnet_info *vi, u16 queue_pairs)

 static int virtnet_close(struct net_device *dev)  {
+       u8 *promisc_allmulti  __free(kfree) = NULL;
        struct virtnet_info *vi = netdev_priv(dev);
        int i;

@@ -2888,6 +2885,7 @@ static void virtnet_rx_mode_work(struct

work_struct *work)

quoted

        struct scatterlist sg[2];
        struct virtio_net_ctrl_mac *mac_data;
        struct netdev_hw_addr *ha;
+       u8 *promisc_allmulti;
        int uc_count;
        int mc_count;
        void *buf;

@@ -2899,22 +2897,27 @@ static void virtnet_rx_mode_work(struct

work_struct *work)

        rtnl_lock();

-       vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
-       vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+       promisc_allmulti = kzalloc(sizeof(*promisc_allmulti), GFP_ATOMIC);
+       if (!promisc_allmulti) {

There is a missing rtnl_unlock() here ?

Yes, you're right. Will send a patch soon.

quoted

+               dev_warn(&dev->dev, "Failed to set RX mode, no memory.\n");
+               return;
+       }