search for: __netif_subqueue_stopped

...e ring and free the skb immediately, this can cause use after free. Fix by storing xmit_more in a local variable. Cc: David S. Miller <davem at davemloft.net> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- David, am I using the API correctly? Seems to work for me. You used __netif_subqueue_stopped but that seems to use a slightly more expensive test_bit internally. The reason I added a variable for the txq here is because it's handy for BQL patch later on. drivers/net/virtio_net.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/virtio_net.c b/drivers...

...e ring and free the skb immediately, this can cause use after free. Fix by storing xmit_more in a local variable. Cc: David S. Miller <davem at davemloft.net> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- David, am I using the API correctly? Seems to work for me. You used __netif_subqueue_stopped but that seems to use a slightly more expensive test_bit internally. The reason I added a variable for the txq here is because it's handy for BQL patch later on. drivers/net/virtio_net.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/virtio_net.c b/drivers...

...-- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -993,12 +993,16 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) virtqueue_disable_cb(sq->vq); } } - } else if (virtqueue_enable_cb(sq->vq)) { - free_old_xmit_skbs(sq, qsize); } - if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) + if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) { virtqueue_kick(sq->vq); + if (sq->vq->num_free >= 2 +MAX_SKB_FRAGS && + unlikely(virtqueue_enable_cb_avail(sq->vq))) { + free_old_xmit_skbs(sq, qsize); + virtqueu...

...et/virtio_net.c > @@ -993,12 +993,16 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > virtqueue_disable_cb(sq->vq); > } > } > - } else if (virtqueue_enable_cb(sq->vq)) { > - free_old_xmit_skbs(sq, qsize); > } > > - if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) > + if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) { > virtqueue_kick(sq->vq); > + if (sq->vq->num_free >= 2 +MAX_SKB_FRAGS && > + unlikely(virtqueue_enable_cb_avail(sq->vq))) { > + free_old_xmit_skbs...

...e use after free. > > Fix by storing xmit_more in a local variable. > > Cc: David S. Miller <davem at davemloft.net> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > David, am I using the API correctly? > Seems to work for me. > You used __netif_subqueue_stopped but that seems to use > a slightly more expensive test_bit internally. > The reason I added a variable for the txq here is because it's handy for > BQL patch later on. > > > drivers/net/virtio_net.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > di...

...e use after free. > > Fix by storing xmit_more in a local variable. > > Cc: David S. Miller <davem at davemloft.net> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > David, am I using the API correctly? > Seems to work for me. > You used __netif_subqueue_stopped but that seems to use > a slightly more expensive test_bit internally. > The reason I added a variable for the txq here is because it's handy for > BQL patch later on. > > > drivers/net/virtio_net.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > di...

...ix by storing xmit_more in a local variable. >> >> Cc: David S. Miller <davem at davemloft.net> >> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> >> --- >> >> David, am I using the API correctly? >> Seems to work for me. >> You used __netif_subqueue_stopped but that seems to use >> a slightly more expensive test_bit internally. >> The reason I added a variable for the txq here is because it's handy for >> BQL patch later on. >> >> >> drivers/net/virtio_net.c | 4 +++- >> 1 file changed, 3 insertions(+), 1...

...-- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -993,12 +993,16 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) virtqueue_disable_cb(sq->vq); } } - } else if (virtqueue_enable_cb(sq->vq)) { - free_old_xmit_skbs(sq, qsize); } - if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) + if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) { virtqueue_kick(sq->vq); + if (sq->vq->num_free >= 2 +MAX_SKB_FRAGS && + unlikely(virtqueue_enable_cb_avail(sq->vq))) { + free_old_xmit_skbs(sq, qsize); + virtqueu...

...ix by storing xmit_more in a local variable. >> >> Cc: David S. Miller <davem at davemloft.net> >> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> >> --- >> >> David, am I using the API correctly? >> Seems to work for me. >> You used __netif_subqueue_stopped but that seems to use >> a slightly more expensive test_bit internally. >> The reason I added a variable for the txq here is because it's handy for >> BQL patch later on. >> >> >> drivers/net/virtio_net.c | 4 +++- >> 1 file changed, 3 insertions(+), 1...

According to David, proper accounting and queueing (at all levels, not just TCP sockets) is more important than trying to skim a bunch of cycles by avoiding TX interrupts. Having an event to free the SKB is absolutely essential for the stack to operate correctly. This series tries to enable tx interrupt for virtio-net. The idea is simple: enable tx interrupt and schedule a tx napi to free old

According to David, proper accounting and queueing (at all levels, not just TCP sockets) is more important than trying to skim a bunch of cycles by avoiding TX interrupts. Having an event to free the SKB is absolutely essential for the stack to operate correctly. This series tries to enable tx interrupt for virtio-net. The idea is simple: enable tx interrupt and schedule a tx napi to free old

From: "Michael S. Tsirkin" <mst at redhat.com> Date: Wed, 15 Oct 2014 16:23:28 +0300 > You used __netif_subqueue_stopped but that seems to use > a slightly more expensive test_bit internally. More expensive in what sense? It should be roughly the same as "x & y" sans the volatile. Anyways I'm ambivalent and I want to see this bug fixes, so I'll apply your patch. Thanks!

...s(sq, qsize); > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > netif_start_subqueue(dev, qnum); > virtqueue_disable_cb(sq->vq); > } > } > + } else if (virtqueue_enable_cb(sq->vq)) { > + free_old_xmit_skbs(sq, qsize); > } > > if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) > @@ -1141,8 +1176,10 @@ static int virtnet_close(struct net_device *dev) > /* Make sure refill_work doesn't re-enable napi! */ > cancel_delayed_work_sync(&vi->refill); > > - for (i = 0; i < vi->max_queue_pairs; i++) > + for...

...size); > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > netif_start_subqueue(dev, qnum); > virtqueue_disable_cb(sq->vq); > } > } > + } else if (virtqueue_enable_cb_urgent(sq->vq)) { > + free_old_xmit_skbs(sq, qsize); > } > > if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) > @@ -1132,8 +1214,10 @@ static int virtnet_close(struct net_device *dev) > /* Make sure refill_work doesn't re-enable napi! */ > cancel_delayed_work_sync(&vi->refill); > > - for (i = 0; i < vi->max_queue_pairs; i++) > + for...

...- free_old_xmit_skbs(sq); + free_old_xmit_skbs(sq, qsize); if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); } } + } else if (virtqueue_enable_cb(sq->vq)) { + free_old_xmit_skbs(sq, qsize); } if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) @@ -1141,8 +1176,10 @@ static int virtnet_close(struct net_device *dev) /* Make sure refill_work doesn't re-enable napi! */ cancel_delayed_work_sync(&vi->refill); - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_que...

...- free_old_xmit_skbs(sq); + free_old_xmit_skbs(sq, qsize); if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); } } + } else if (virtqueue_enable_cb(sq->vq)) { + free_old_xmit_skbs(sq, qsize); } if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) @@ -1141,8 +1176,10 @@ static int virtnet_close(struct net_device *dev) /* Make sure refill_work doesn't re-enable napi! */ cancel_delayed_work_sync(&vi->refill); - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_que...

...ee_old_xmit_skbs(sq); + free_old_xmit_skbs(sq, qsize); if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); } } + } else if (virtqueue_enable_cb_urgent(sq->vq)) { + free_old_xmit_skbs(sq, qsize); } if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) @@ -1132,8 +1214,10 @@ static int virtnet_close(struct net_device *dev) /* Make sure refill_work doesn't re-enable napi! */ cancel_delayed_work_sync(&vi->refill); - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_que...

...ee_old_xmit_skbs(sq); + free_old_xmit_skbs(sq, qsize); if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); } } + } else if (virtqueue_enable_cb_urgent(sq->vq)) { + free_old_xmit_skbs(sq, qsize); } if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) @@ -1132,8 +1214,10 @@ static int virtnet_close(struct net_device *dev) /* Make sure refill_work doesn't re-enable napi! */ cancel_delayed_work_sync(&vi->refill); - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_que...

Hello all: We free old transmitted packets in ndo_start_xmit() currently, so any packet must be orphaned also there. This was used to reduce the overhead of tx interrupt to achieve better performance. But this may not work for some protocols such as TCP stream. TCP depends on the value of sk_wmem_alloc to implement various optimization for small packets stream such as TCP small queue and auto

Hello all: We free old transmitted packets in ndo_start_xmit() currently, so any packet must be orphaned also there. This was used to reduce the overhead of tx interrupt to achieve better performance. But this may not work for some protocols such as TCP stream. TCP depends on the value of sk_wmem_alloc to implement various optimization for small packets stream such as TCP small queue and auto