When sending control commands, virtio net sets up several buffers for
DMA. The buffers are all part of the net device which means it's
actually allocated by kvmalloc so in theory (on extreme memory pressure)
it's possible to get a vmalloc'ed buffer which on some platforms means
we can't DMA there.
Fix up by moving the DMA buffers out into a separate structure.
Reported-by: Mikulas Patocka <mpatocka at redhat.com>
Suggested-by: Eric Dumazet <eric.dumazet at gmail.com>
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
Lightly tested.
Mikulas, does this address your problem?
drivers/net/virtio_net.c | 68 +++++++++++++++++++++++++++---------------------
1 file changed, 39 insertions(+), 29 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b187ec..82f50e5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -147,6 +147,17 @@ struct receive_queue {
struct xdp_rxq_info xdp_rxq;
};
+/* Control VQ buffers: protected by the rtnl lock */
+struct control_buf {
+ struct virtio_net_ctrl_hdr hdr;
+ virtio_net_ctrl_ack status;
+ struct virtio_net_ctrl_mq mq;
+ u8 promisc;
+ u8 allmulti;
+ u16 vid;
+ u64 offloads;
+};
+
struct virtnet_info {
struct virtio_device *vdev;
struct virtqueue *cvq;
@@ -192,14 +203,7 @@ struct virtnet_info {
struct hlist_node node;
struct hlist_node node_dead;
- /* Control VQ buffers: protected by the rtnl lock */
- struct virtio_net_ctrl_hdr ctrl_hdr;
- virtio_net_ctrl_ack ctrl_status;
- struct virtio_net_ctrl_mq ctrl_mq;
- u8 ctrl_promisc;
- u8 ctrl_allmulti;
- u16 ctrl_vid;
- u64 ctrl_offloads;
+ struct control_buf *ctrl;
/* Ethtool settings */
u8 duplex;
@@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info
*vi, u8 class, u8 cmd,
/* Caller should know better */
BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
- vi->ctrl_status = ~0;
- vi->ctrl_hdr.class = class;
- vi->ctrl_hdr.cmd = cmd;
+ vi->ctrl->status = ~0;
+ vi->ctrl->hdr.class = class;
+ vi->ctrl->hdr.cmd = cmd;
/* Add header */
- sg_init_one(&hdr, &vi->ctrl_hdr, sizeof(vi->ctrl_hdr));
+ sg_init_one(&hdr, &vi->ctrl->hdr, sizeof(vi->ctrl->hdr));
sgs[out_num++] = &hdr;
if (out)
sgs[out_num++] = out;
/* Add return status. */
- sg_init_one(&stat, &vi->ctrl_status, sizeof(vi->ctrl_status));
+ sg_init_one(&stat, &vi->ctrl->status,
sizeof(vi->ctrl->status));
sgs[out_num] = &stat;
BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
if (unlikely(!virtqueue_kick(vi->cvq)))
- return vi->ctrl_status == VIRTIO_NET_OK;
+ return vi->ctrl->status == VIRTIO_NET_OK;
/* Spin for a response, the kick causes an ioport write, trapping
* into the hypervisor, so the request should be handled immediately.
@@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info *vi,
u8 class, u8 cmd,
!virtqueue_is_broken(vi->cvq))
cpu_relax();
- return vi->ctrl_status == VIRTIO_NET_OK;
+ return vi->ctrl->status == VIRTIO_NET_OK;
}
static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi,
u16 queue_pairs)
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);
- sg_init_one(&sg, &vi->ctrl_mq, sizeof(vi->ctrl_mq));
+ vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev,
queue_pairs);
+ sg_init_one(&sg, &vi->ctrl->mq, sizeof(vi->ctrl->mq));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg)) {
@@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
return;
- vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
- vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+ vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
+ vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
- sg_init_one(sg, &vi->ctrl_promisc, sizeof(vi->ctrl_promisc));
+ sg_init_one(sg, &vi->ctrl->promisc,
sizeof(vi->ctrl->promisc));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
VIRTIO_NET_CTRL_RX_PROMISC, sg))
dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
- vi->ctrl_promisc ? "en" : "dis");
+ vi->ctrl->promisc ? "en" : "dis");
- sg_init_one(sg, &vi->ctrl_allmulti, sizeof(vi->ctrl_allmulti));
+ sg_init_one(sg, &vi->ctrl->allmulti,
sizeof(vi->ctrl->allmulti));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
- vi->ctrl_allmulti ? "en" : "dis");
+ vi->ctrl->allmulti ? "en" : "dis");
uc_count = netdev_uc_count(dev);
mc_count = netdev_mc_count(dev);
@@ -1714,8 +1718,8 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev,
struct virtnet_info *vi = netdev_priv(dev);
struct scatterlist sg;
- vi->ctrl_vid = vid;
- sg_init_one(&sg, &vi->ctrl_vid, sizeof(vi->ctrl_vid));
+ vi->ctrl->vid = vid;
+ sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
VIRTIO_NET_CTRL_VLAN_ADD, &sg))
@@ -1729,8 +1733,8 @@ static int virtnet_vlan_rx_kill_vid(struct net_device
*dev,
struct virtnet_info *vi = netdev_priv(dev);
struct scatterlist sg;
- vi->ctrl_vid = vid;
- sg_init_one(&sg, &vi->ctrl_vid, sizeof(vi->ctrl_vid));
+ vi->ctrl->vid = vid;
+ sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
VIRTIO_NET_CTRL_VLAN_DEL, &sg))
@@ -2126,9 +2130,9 @@ static int virtnet_restore_up(struct virtio_device *vdev)
static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
{
struct scatterlist sg;
- vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads);
+ vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads);
- sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads));
+ sg_init_one(&sg, &vi->ctrl->offloads,
sizeof(vi->ctrl->offloads));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
@@ -2351,6 +2355,7 @@ static void virtnet_free_queues(struct virtnet_info *vi)
kfree(vi->rq);
kfree(vi->sq);
+ kfree(vi->err_ctrl);
}
static void _free_receive_bufs(struct virtnet_info *vi)
@@ -2543,6 +2548,9 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
{
int i;
+ vi->ctrl = kzalloc(sizeof(*vi->ctrl), GFP_KERNEL);
+ if (!vi->ctrl)
+ goto err_ctrl;
vi->sq = kzalloc(sizeof(*vi->sq) * vi->max_queue_pairs, GFP_KERNEL);
if (!vi->sq)
goto err_sq;
@@ -2571,6 +2579,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
err_rq:
kfree(vi->sq);
err_sq:
+ kfree(vq->ctrl);
+err_ctrl:
return -ENOMEM;
}
--
MST
On Thu, Apr 19, 2018 at 08:01:49AM +0300, Michael S. Tsirkin wrote:> When sending control commands, virtio net sets up several buffers for > DMA. The buffers are all part of the net device which means it's > actually allocated by kvmalloc so in theory (on extreme memory pressure) > it's possible to get a vmalloc'ed buffer which on some platforms means > we can't DMA there. > > Fix up by moving the DMA buffers out into a separate structure. > > Reported-by: Mikulas Patocka <mpatocka at redhat.com> > Suggested-by: Eric Dumazet <eric.dumazet at gmail.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>Ouch forgot to commit a fix. Pls ignore will send v2 now.> --- > > Lightly tested. > Mikulas, does this address your problem? > > drivers/net/virtio_net.c | 68 +++++++++++++++++++++++++++--------------------- > 1 file changed, 39 insertions(+), 29 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 7b187ec..82f50e5 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -147,6 +147,17 @@ struct receive_queue { > struct xdp_rxq_info xdp_rxq; > }; > > +/* Control VQ buffers: protected by the rtnl lock */ > +struct control_buf { > + struct virtio_net_ctrl_hdr hdr; > + virtio_net_ctrl_ack status; > + struct virtio_net_ctrl_mq mq; > + u8 promisc; > + u8 allmulti; > + u16 vid; > + u64 offloads; > +}; > + > struct virtnet_info { > struct virtio_device *vdev; > struct virtqueue *cvq; > @@ -192,14 +203,7 @@ struct virtnet_info { > struct hlist_node node; > struct hlist_node node_dead; > > - /* Control VQ buffers: protected by the rtnl lock */ > - struct virtio_net_ctrl_hdr ctrl_hdr; > - virtio_net_ctrl_ack ctrl_status; > - struct virtio_net_ctrl_mq ctrl_mq; > - u8 ctrl_promisc; > - u8 ctrl_allmulti; > - u16 ctrl_vid; > - u64 ctrl_offloads; > + struct control_buf *ctrl; > > /* Ethtool settings */ > u8 duplex; > @@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > /* Caller should know better */ > BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)); > > - vi->ctrl_status = ~0; > - vi->ctrl_hdr.class = class; > - vi->ctrl_hdr.cmd = cmd; > + vi->ctrl->status = ~0; > + vi->ctrl->hdr.class = class; > + vi->ctrl->hdr.cmd = cmd; > /* Add header */ > - sg_init_one(&hdr, &vi->ctrl_hdr, sizeof(vi->ctrl_hdr)); > + sg_init_one(&hdr, &vi->ctrl->hdr, sizeof(vi->ctrl->hdr)); > sgs[out_num++] = &hdr; > > if (out) > sgs[out_num++] = out; > > /* Add return status. */ > - sg_init_one(&stat, &vi->ctrl_status, sizeof(vi->ctrl_status)); > + sg_init_one(&stat, &vi->ctrl->status, sizeof(vi->ctrl->status)); > sgs[out_num] = &stat; > > BUG_ON(out_num + 1 > ARRAY_SIZE(sgs)); > virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC); > > if (unlikely(!virtqueue_kick(vi->cvq))) > - return vi->ctrl_status == VIRTIO_NET_OK; > + return vi->ctrl->status == VIRTIO_NET_OK; > > /* Spin for a response, the kick causes an ioport write, trapping > * into the hypervisor, so the request should be handled immediately. > @@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > !virtqueue_is_broken(vi->cvq)) > cpu_relax(); > > - return vi->ctrl_status == VIRTIO_NET_OK; > + return vi->ctrl->status == VIRTIO_NET_OK; > } > > static int virtnet_set_mac_address(struct net_device *dev, void *p) > @@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs) > 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); > - sg_init_one(&sg, &vi->ctrl_mq, sizeof(vi->ctrl_mq)); > + vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs); > + sg_init_one(&sg, &vi->ctrl->mq, sizeof(vi->ctrl->mq)); > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ, > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg)) { > @@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device *dev) > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > return; > > - vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0); > - vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > + vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > + vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > - sg_init_one(sg, &vi->ctrl_promisc, sizeof(vi->ctrl_promisc)); > + sg_init_one(sg, &vi->ctrl->promisc, sizeof(vi->ctrl->promisc)); > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX, > VIRTIO_NET_CTRL_RX_PROMISC, sg)) > dev_warn(&dev->dev, "Failed to %sable promisc mode.\n", > - vi->ctrl_promisc ? "en" : "dis"); > + vi->ctrl->promisc ? "en" : "dis"); > > - sg_init_one(sg, &vi->ctrl_allmulti, sizeof(vi->ctrl_allmulti)); > + sg_init_one(sg, &vi->ctrl->allmulti, sizeof(vi->ctrl->allmulti)); > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX, > VIRTIO_NET_CTRL_RX_ALLMULTI, sg)) > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > - vi->ctrl_allmulti ? "en" : "dis"); > + vi->ctrl->allmulti ? "en" : "dis"); > > uc_count = netdev_uc_count(dev); > mc_count = netdev_mc_count(dev); > @@ -1714,8 +1718,8 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev, > struct virtnet_info *vi = netdev_priv(dev); > struct scatterlist sg; > > - vi->ctrl_vid = vid; > - sg_init_one(&sg, &vi->ctrl_vid, sizeof(vi->ctrl_vid)); > + vi->ctrl->vid = vid; > + sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid)); > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN, > VIRTIO_NET_CTRL_VLAN_ADD, &sg)) > @@ -1729,8 +1733,8 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, > struct virtnet_info *vi = netdev_priv(dev); > struct scatterlist sg; > > - vi->ctrl_vid = vid; > - sg_init_one(&sg, &vi->ctrl_vid, sizeof(vi->ctrl_vid)); > + vi->ctrl->vid = vid; > + sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid)); > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN, > VIRTIO_NET_CTRL_VLAN_DEL, &sg)) > @@ -2126,9 +2130,9 @@ static int virtnet_restore_up(struct virtio_device *vdev) > static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads) > { > struct scatterlist sg; > - vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads); > + vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads); > > - sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads)); > + sg_init_one(&sg, &vi->ctrl->offloads, sizeof(vi->ctrl->offloads)); > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS, > VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) { > @@ -2351,6 +2355,7 @@ static void virtnet_free_queues(struct virtnet_info *vi) > > kfree(vi->rq); > kfree(vi->sq); > + kfree(vi->err_ctrl); > } > > static void _free_receive_bufs(struct virtnet_info *vi) > @@ -2543,6 +2548,9 @@ static int virtnet_alloc_queues(struct virtnet_info *vi) > { > int i; > > + vi->ctrl = kzalloc(sizeof(*vi->ctrl), GFP_KERNEL); > + if (!vi->ctrl) > + goto err_ctrl; > vi->sq = kzalloc(sizeof(*vi->sq) * vi->max_queue_pairs, GFP_KERNEL); > if (!vi->sq) > goto err_sq; > @@ -2571,6 +2579,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi) > err_rq: > kfree(vi->sq); > err_sq: > + kfree(vq->ctrl); > +err_ctrl: > return -ENOMEM; > } > > -- > MST
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on net/master]
url:
https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/virtio_net-split-out-ctrl-buffer/20180419-145754
config: x86_64-randconfig-x006-201815 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers//net/virtio_net.c: In function
'virtnet_free_queues':>> drivers//net/virtio_net.c:2365:12: error: 'struct virtnet_info'
has no member named 'err_ctrl'; did you mean 'ctrl'?
kfree(vi->err_ctrl);
^~~~~~~~
ctrl
drivers//net/virtio_net.c: In function
'virtnet_alloc_queues':>> drivers//net/virtio_net.c:2589:8: error: 'vq' undeclared (first
use in this function); did you mean 'vi'?
kfree(vq->ctrl);
^~
vi
drivers//net/virtio_net.c:2589:8: note: each undeclared identifier is
reported only once for each function it appears in
vim +2365 drivers//net/virtio_net.c
2347
2348 static void virtnet_free_queues(struct virtnet_info *vi)
2349 {
2350 int i;
2351
2352 for (i = 0; i < vi->max_queue_pairs; i++) {
2353 napi_hash_del(&vi->rq[i].napi);
2354 netif_napi_del(&vi->rq[i].napi);
2355 netif_napi_del(&vi->sq[i].napi);
2356 }
2357
2358 /* We called napi_hash_del() before netif_napi_del(),
2359 * we need to respect an RCU grace period before freeing vi->rq
2360 */
2361 synchronize_net();
2362
2363 kfree(vi->rq);
2364 kfree(vi->sq);> 2365 kfree(vi->err_ctrl);
2366 }
2367
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 27230 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20180419/7a1edf7f/attachment-0001.bin>
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on net/master]
url:
https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/virtio_net-split-out-ctrl-buffer/20180419-145754
config: i386-randconfig-a0-201815 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/virtio_net.c: In function
'virtnet_free_queues':>> drivers/net/virtio_net.c:2365:10: error: 'struct virtnet_info'
has no member named 'err_ctrl'
kfree(vi->err_ctrl);
^
drivers/net/virtio_net.c: In function
'virtnet_alloc_queues':>> drivers/net/virtio_net.c:2589:8: error: 'vq' undeclared (first
use in this function)
kfree(vq->ctrl);
^
drivers/net/virtio_net.c:2589:8: note: each undeclared identifier is reported
only once for each function it appears in
vim +2365 drivers/net/virtio_net.c
2347
2348 static void virtnet_free_queues(struct virtnet_info *vi)
2349 {
2350 int i;
2351
2352 for (i = 0; i < vi->max_queue_pairs; i++) {
2353 napi_hash_del(&vi->rq[i].napi);
2354 netif_napi_del(&vi->rq[i].napi);
2355 netif_napi_del(&vi->sq[i].napi);
2356 }
2357
2358 /* We called napi_hash_del() before netif_napi_del(),
2359 * we need to respect an RCU grace period before freeing vi->rq
2360 */
2361 synchronize_net();
2362
2363 kfree(vi->rq);
2364 kfree(vi->sq);> 2365 kfree(vi->err_ctrl);
2366 }
2367
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 31332 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20180419/4e9c9952/attachment-0001.bin>
Possibly Parallel Threads
- [PATCH v2 net 1/3] virtio_net: split out ctrl buffer
- [PATCH v4 1/6] virtio-net: Stop doing DMA from the stack
- [PATCH v2 1/3] virtio_net: Stop doing DMA from the stack
- [PATCH v4 1/6] virtio-net: Stop doing DMA from the stack
- [PATCH v2 1/3] virtio_net: Stop doing DMA from the stack