search for: skb_orphan

Various drivers do skb_orphan() because they do not free transmitted skbs in a timely manner (causing apps which hit their socket limits to stall, socket close to hang, etc.). DaveM points out that there are advantages to doing it generally (it's more likely to be on same CPU than after xmit), and I couldn't find any n...

Various drivers do skb_orphan() because they do not free transmitted skbs in a timely manner (causing apps which hit their socket limits to stall, socket close to hang, etc.). DaveM points out that there are advantages to doing it generally (it's more likely to be on same CPU than after xmit), and I couldn't find any n...

...s available per socket, not per ether device, so that different protocols don't run into a cumulative traffic barrier. Fix. Should we agree that the observed behaviour should indeed be amended, i propose the following fix: in br_device.c, function br_dev_xmit() (or __br_dev_xmit() in 2.4): skb_orphan() the sk_buff to be delivered before handing it to the clone loop in br_flood_deliver(), calling br_flood(). skb_orphan() disassociates the sk_buff from its owning socket and runs the "destructor" attached to the sk_buff, which restores the wmem_alloc quota of the sending socket. If the...

...f *skb, struct net_device *dev) > } > } > > - if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) > + if (kick || netif_xmit_stopped(txq)) > virtqueue_kick(sq->vq); > > return NETDEV_TX_OK; I must say I am kind of confused by this patch. Why the skb_orphan(skb) & nf_reset(skb) do not have the same issue ? It looks like following patch is needed ? diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index ec2a8b41ed41..17cc42c6a559 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -927,6 +927,10 @@ static netde...

We free transmitted packets in ndo_start_xmit() in the past to get better performance in the past. One side effect is that skb_orphan() needs to be called in ndo_start_xmit() which makes sk_wmem_alloc not accurate in fact. For TCP protocol, this means several optimization could not work well such as TCP small queue and auto corking. This can lead extra low throughput of small packets stream. Thanks to the urgent descriptor suppo...

We free transmitted packets in ndo_start_xmit() in the past to get better performance in the past. One side effect is that skb_orphan() needs to be called in ndo_start_xmit() which makes sk_wmem_alloc not accurate in fact. For TCP protocol, this means several optimization could not work well such as TCP small queue and auto corking. This can lead extra low throughput of small packets stream. Thanks to the urgent descriptor suppo...

commit 0b725a2ca61bedc33a2a63d0451d528b268cf975 net: Remove ndo_xmit_flush netdev operation, use signalling instead. added code that looks at skb->xmit_more after the skb has been put in TX VQ. Since some paths process the 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>

commit 0b725a2ca61bedc33a2a63d0451d528b268cf975 net: Remove ndo_xmit_flush netdev operation, use signalling instead. added code that looks at skb->xmit_more after the skb has been put in TX VQ. Since some paths process the 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>

On Sat, Oct 11, 2014 at 03:16:46PM +0800, Jason Wang wrote: > We free transmitted packets in ndo_start_xmit() in the past to get better > performance in the past. One side effect is that skb_orphan() needs to be > called in ndo_start_xmit() which makes sk_wmem_alloc not accurate in > fact. For TCP protocol, this means several optimization could not work well > such as TCP small queue and auto corking. This can lead extra low > throughput of small packets stream. > > Thanks...

...can happen with OOM and indirect buffers. */ @@ -532,8 +531,11 @@ again: } return NETDEV_TX_BUSY; } - vi->svq->vq_ops->kick(vi->svq); + + /* Put new one in send queue */ + __skb_queue_head(&vi->send, skb); + /* Don't wait up for transmitted skbs to be freed. */ skb_orphan(skb); nf_reset(skb); -- 1.6.5.rc2

...can happen with OOM and indirect buffers. */ @@ -532,8 +531,11 @@ again: } return NETDEV_TX_BUSY; } - vi->svq->vq_ops->kick(vi->svq); + + /* Put new one in send queue */ + __skb_queue_head(&vi->send, skb); + /* Don't wait up for transmitted skbs to be freed. */ skb_orphan(skb); nf_reset(skb); -- 1.6.5.rc2

...op and what the impacts are. Signed-off-by: Stephen Hemminger <stephen at networkplumber.org> --- a/drivers/net/virtio_net.c 2015-03-24 15:20:25.174671000 -0700 +++ b/drivers/net/virtio_net.c 2015-03-24 16:17:28.478525333 -0700 @@ -939,8 +939,12 @@ static netdev_tx_t start_xmit(struct sk_ skb_orphan(skb); nf_reset(skb); - /* Apparently nice girls don't return TX_BUSY; stop the queue - * before it gets out of hand. Naturally, this wastes entries. */ + /* It is better to stop queue if running out of space + * instead of forcing queuing layer to requeue the skb + * by returning TX_BUS...

...op and what the impacts are. Signed-off-by: Stephen Hemminger <stephen at networkplumber.org> --- a/drivers/net/virtio_net.c 2015-03-24 15:20:25.174671000 -0700 +++ b/drivers/net/virtio_net.c 2015-03-24 16:17:28.478525333 -0700 @@ -939,8 +939,12 @@ static netdev_tx_t start_xmit(struct sk_ skb_orphan(skb); nf_reset(skb); - /* Apparently nice girls don't return TX_BUSY; stop the queue - * before it gets out of hand. Naturally, this wastes entries. */ + /* It is better to stop queue if running out of space + * instead of forcing queuing layer to requeue the skb + * by returning TX_BUS...

...errors++; if (net_ratelimit()) dev_warn(&dev->dev, @@ -886,6 +885,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) kfree_skb(skb); return NETDEV_TX_OK; } + virtqueue_kick(sq->vq); /* Don't wait up for transmitted skbs to be freed. */ skb_orphan(skb); -- 1.7.1

...errors++; if (net_ratelimit()) dev_warn(&dev->dev, @@ -886,6 +885,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) kfree_skb(skb); return NETDEV_TX_OK; } + virtqueue_kick(sq->vq); /* Don't wait up for transmitted skbs to be freed. */ skb_orphan(skb); -- 1.7.1

...t; + free_old_xmit_skbs(sq); > > /* timestamp packet in software */ > skb_tx_timestamp(skb); > @@ -1152,8 +1183,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > } > > /* Don't wait up for transmitted skbs to be freed. */ > - skb_orphan(skb); > - nf_reset(skb); > + if (!use_napi) { > + skb_orphan(skb); > + nf_reset(skb); > + } > > /* If running out of space, stop queue to avoid getting packets that we > * are then unable to transmit. > @@ -1167,7 +1200,8 @@ static netdev_tx_t start_xmit(struct s...

...; + bool use_napi = sq->napi.weight; /* Free up any pending old buffers before queueing new ones. */ free_old_xmit_skbs(sq); @@ -1152,8 +1197,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) } /* Don't wait up for transmitted skbs to be freed. */ - skb_orphan(skb); - nf_reset(skb); + if (!use_napi) { + skb_orphan(skb); + nf_reset(skb); + } /* If running out of space, stop queue to avoid getting packets that we * are then unable to transmit. @@ -1167,7 +1214,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) */...

...(sq->vq); > > /* Try to transmit */ > err = xmit_skb(sq, skb); > @@ -941,27 +981,19 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > return NETDEV_TX_OK; > } > > - /* Don't wait up for transmitted skbs to be freed. */ > - skb_orphan(skb); > - nf_reset(skb); > - > /* Apparently nice girls don't return TX_BUSY; stop the queue > * before it gets out of hand. Naturally, this wastes entries. */ > - if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > + if (sq->vq->num_free < 2+MAX_SKB_FRAGS) &...

...(sq->vq); > > /* Try to transmit */ > err = xmit_skb(sq, skb); > @@ -941,27 +981,19 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > return NETDEV_TX_OK; > } > > - /* Don't wait up for transmitted skbs to be freed. */ > - skb_orphan(skb); > - nf_reset(skb); > - > /* Apparently nice girls don't return TX_BUSY; stop the queue > * before it gets out of hand. Naturally, this wastes entries. */ > - if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > + if (sq->vq->num_free < 2+MAX_SKB_FRAGS) &...

...- free_old_xmit_skbs(sq); + if (!use_napi) + free_old_xmit_skbs(sq); /* timestamp packet in software */ skb_tx_timestamp(skb); @@ -1152,8 +1183,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) } /* Don't wait up for transmitted skbs to be freed. */ - skb_orphan(skb); - nf_reset(skb); + if (!use_napi) { + skb_orphan(skb); + nf_reset(skb); + } /* If running out of space, stop queue to avoid getting packets that we * are then unable to transmit. @@ -1167,7 +1200,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) */...