search for: skb_xmit_don

...TOH if often enter this branch tx queue maybe stopped. > } and changed it here so it won't restart queue if host consumed all buffers. unsurprisingly this makes driver not work. > > } > > //########## Should we start queue here? I found that sometimes skb_xmit_done run before netif_stop_queue if this occurred the queue's state is > //########## stopped and have to reload virtio-net module to restore network. With or without your changes? Is this the condition you describe? if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { ---> at thi...

...TOH if often enter this branch tx queue maybe stopped. > } and changed it here so it won't restart queue if host consumed all buffers. unsurprisingly this makes driver not work. > > } > > //########## Should we start queue here? I found that sometimes skb_xmit_done run before netif_stop_queue if this occurred the queue's state is > //########## stopped and have to reload virtio-net module to restore network. With or without your changes? Is this the condition you describe? if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { ---> at thi...

...le value 0 means not. To properly synchronize with the data path, tx napi is disabled and tx lock is held when changing the value of napi weight. And two more places that can access tx napi weight: - speculative tx polling in rx napi, we can leave it as is since it not a must for correctness. - skb_xmit_done(), one more check of napi weight is added before trying to enable tx to avoid tx to be disabled forever if napi is disabled after skb_xmit_done() but before the napi Link: https://patchwork.ozlabs.org/patch/948149/ Suggested-by: Jason Wang <jasowang at redhat.com> Signed-off-by: Willem...

...le value 0 means not. To properly synchronize with the data path, tx napi is disabled and tx lock is held when changing the value of napi weight. And two more places that can access tx napi weight: - speculative tx polling in rx napi, we can leave it as is since it not a must for correctness. - skb_xmit_done(), one more check of napi weight is added before trying to enable tx to avoid tx to be disabled forever if napi is disabled after skb_xmit_done() but before the napi Link: https://patchwork.ozlabs.org/patch/948149/ Suggested-by: Jason Wang <jasowang at redhat.com> Signed-off-by: Willem...

On 10/15/2014 07:11 AM, Michael S. Tsirkin wrote: > On Wed, Oct 15, 2014 at 12:53:59AM +0300, Michael S. Tsirkin wrote: >> > static void skb_xmit_done(struct virtqueue *vq) >> > { >> > struct virtnet_info *vi = vq->vdev->priv; >> > + struct send_queue *sq = &vi->sq[vq2txq(vq)]; >> > >> > - /* Suppress further interrupts. */ >> > - virtqueue_disable_cb(vq); >> > - &g...

On 10/15/2014 07:11 AM, Michael S. Tsirkin wrote: > On Wed, Oct 15, 2014 at 12:53:59AM +0300, Michael S. Tsirkin wrote: >> > static void skb_xmit_done(struct virtqueue *vq) >> > { >> > struct virtnet_info *vi = vq->vdev->priv; >> > + struct send_queue *sq = &vi->sq[vq2txq(vq)]; >> > >> > - /* Suppress further interrupts. */ >> > - virtqueue_disable_cb(vq); >> > - &g...

...pped. >> } > > and changed it here so it won't restart queue if host consumed > all buffers. > unsurprisingly this makes driver not work. > > >> >> } >> >> //########## Should we start queue here? I found that sometimes skb_xmit_done run before netif_stop_queue if this occurred the queue's state is >> //########## stopped and have to reload virtio-net module to restore network. > > With or without your changes? without > Is this the condition you describe? > > > if (sq->vq->num_...

...pped. >> } > > and changed it here so it won't restart queue if host consumed > all buffers. > unsurprisingly this makes driver not work. > > >> >> } >> >> //########## Should we start queue here? I found that sometimes skb_xmit_done run before netif_stop_queue if this occurred the queue's state is >> //########## stopped and have to reload virtio-net module to restore network. > > With or without your changes? without > Is this the condition you describe? > > > if (sq->vq->num_...

...ed it here so it won't restart queue if host consumed > > all buffers. > > unsurprisingly this makes driver not work. > > > > > >> > >> } > >> > >> //########## Should we start queue here? I found that sometimes skb_xmit_done run before netif_stop_queue if this occurred the queue's state is > >> //########## stopped and have to reload virtio-net module to restore network. > > > > With or without your changes? > > without > > > Is this the condition you describe? > > &...

...ed it here so it won't restart queue if host consumed > > all buffers. > > unsurprisingly this makes driver not work. > > > > > >> > >> } > >> > >> //########## Should we start queue here? I found that sometimes skb_xmit_done run before netif_stop_queue if this occurred the queue's state is > >> //########## stopped and have to reload virtio-net module to restore network. > > > > With or without your changes? > > without > > > Is this the condition you describe? > > &...

...perly synchronize with the data path, tx napi is disabled and > tx lock is held when changing the value of napi weight. And two more > places that can access tx napi weight: > > - speculative tx polling in rx napi, we can leave it as is since it > not a must for correctness. > - skb_xmit_done(), one more check of napi weight is added before > trying to enable tx to avoid tx to be disabled forever if napi is > disabled after skb_xmit_done() but before the napi > > Link: https://patchwork.ozlabs.org/patch/948149/ > Suggested-by: Jason Wang <jasowang at redhat.com&gt...

...ULL) { + pr_debug("Sent skb %p\n", skb); + + u64_stats_update_begin(&stats->tx_syncp); + stats->tx_bytes += skb->len; + stats->tx_packets++; + u64_stats_update_end(&stats->tx_syncp); + + dev_kfree_skb_any(skb); + sent++; + } + + return sent; +} + static void skb_xmit_done(struct virtqueue *vq) { struct virtnet_info *vi = vq->vdev->priv; + struct send_queue *sq = &vi->sq[vq2txq(vq)]; - /* Suppress further interrupts. */ - virtqueue_disable_cb(vq); - - /* We were probably waiting for more output buffers. */ - netif_wake_subqueue(vi->dev, vq2txq(v...

...ULL) { + pr_debug("Sent skb %p\n", skb); + + u64_stats_update_begin(&stats->tx_syncp); + stats->tx_bytes += skb->len; + stats->tx_packets++; + u64_stats_update_end(&stats->tx_syncp); + + dev_kfree_skb_any(skb); + sent++; + } + + return sent; +} + static void skb_xmit_done(struct virtqueue *vq) { struct virtnet_info *vi = vq->vdev->priv; + struct send_queue *sq = &vi->sq[vq2txq(vq)]; - /* Suppress further interrupts. */ - virtqueue_disable_cb(vq); - - /* We were probably waiting for more output buffers. */ - netif_wake_subqueue(vi->dev, vq2txq(v...

...> > > interrupt is not disabled actually in this case. > > > > > > Thanks > > > > Only if event_triggered is true, which without event index it never is. > > I'm not sure I will get here. I meant for example the commit > suppresses the effort of skb_xmit_done(): > > static void skb_xmit_done(struct virtqueue *vq) > { > struct virtnet_info *vi = vq->vdev->priv; > struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; > > /* Suppress further interrupts. */ > virtqueue_disable_cb(vq); &g...

On Wed, Oct 15, 2014 at 12:53:59AM +0300, Michael S. Tsirkin wrote: > static void skb_xmit_done(struct virtqueue *vq) > { > struct virtnet_info *vi = vq->vdev->priv; > + struct send_queue *sq = &vi->sq[vq2txq(vq)]; > > - /* Suppress further interrupts. */ > - virtqueue_disable_cb(vq); > - One note here: current code seems racy because of doing virtqueu...

...api, processed) && - unlikely(virtqueue_poll(vq, opaque))) - virtqueue_napi_schedule(napi, vq); + if (napi_complete_done(napi, processed)) { + if (unlikely(virtqueue_poll(vq, opaque))) + virtqueue_napi_schedule(napi, vq); + } else { + virtqueue_disable_cb(vq); + } } static void skb_xmit_done(struct virtqueue *vq) -- 1.8.3.1

...api, processed) && - unlikely(virtqueue_poll(vq, opaque))) - virtqueue_napi_schedule(napi, vq); + if (napi_complete_done(napi, processed)) { + if (unlikely(virtqueue_poll(vq, opaque))) + virtqueue_napi_schedule(napi, vq); + } else { + virtqueue_disable_cb(vq); + } } static void skb_xmit_done(struct virtqueue *vq) -- 1.8.3.1

...chain through xmit_more and a new helper to publish current avail >> idx as used event. >> >> This series fixes several issues of original rfc pointed out by Michael. > Could you list the issues, for ease of review? Probably just one: - Move the virtqueue_disable_cb() from skb_xmit_done() into virtnet_poll_tx() under tx lock.

...chain through xmit_more and a new helper to publish current avail >> idx as used event. >> >> This series fixes several issues of original rfc pointed out by Michael. > Could you list the issues, for ease of review? Probably just one: - Move the virtqueue_disable_cb() from skb_xmit_done() into virtnet_poll_tx() under tx lock.

...k McLoughlin wrote: > virtio_net currently only frees old transmit skbs just > before queueing new ones. If the queue is full, it then > enables interrupts and waits for notification that more > work has been performed. Hi Mark, This patch is fine, but it's better to do it from skb_xmit_done(). Of course, this is usually called from an interrupt handler, so it's not entirely trivial: we can't free the skbs there. A softirq is probably the answer here, but AFAICT that's old fashioned. Not sure what the right way of doing this is now... Rusty.