| Message ID | 200905292346.04815.rusty@rustcorp.com.au |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2009-05-29 at 23:46 +0930, Rusty Russell wrote: > This effectively reverts 99ffc696d10b28580fe93441d627cf290ac4484c > "virtio: wean net driver off NETDEV_TX_BUSY". > > The complexity of queuing an skb (setting a tasklet to re-xmit) is > questionable, It certainly adds some subtle complexities to start_xmit() > especially once we get rid of the other reason for the > tasklet in the next patch. > > If the skb won't fit in the tx queue, just return NETDEV_TX_BUSY. It > might be frowned upon, but it's common and not going away any time > soon. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > --- > drivers/net/virtio_net.c | 49 ++++++++++------------------------------------- > 1 file changed, 11 insertions(+), 38 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > > @@ -526,27 +517,14 @@ again: > /* Free up any pending old buffers before queueing new ones. */ > free_old_xmit_skbs(vi); > > - /* If we has a buffer left over from last time, send it now. */ > - if (unlikely(vi->last_xmit_skb) && > - xmit_skb(vi, vi->last_xmit_skb) != 0) > - goto stop_queue; > + /* Put new one in send queue and do transmit */ > + __skb_queue_head(&vi->send, skb); > + if (likely(xmit_skb(vi, skb) == 0)) { > + vi->svq->vq_ops->kick(vi->svq); > + return NETDEV_TX_OK; > + } Hmm, is it okay to leave the skb on the send queue if we return NETDEV_TX_BUSY? Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 11:46:04PM +0930, Rusty Russell wrote: > > This effectively reverts 99ffc696d10b28580fe93441d627cf290ac4484c > "virtio: wean net driver off NETDEV_TX_BUSY". > > The complexity of queuing an skb (setting a tasklet to re-xmit) is > questionable, especially once we get rid of the other reason for the > tasklet in the next patch. > > If the skb won't fit in the tx queue, just return NETDEV_TX_BUSY. It > might be frowned upon, but it's common and not going away any time > soon. This looks like a step backwards to me. If we have to do this to fix a bug, sure. But just doing it for the sake of it smells wrong. Cheers,
On Tue, 2 Jun 2009 06:35:52 pm Herbert Xu wrote: > On Fri, May 29, 2009 at 11:46:04PM +0930, Rusty Russell wrote: > > This effectively reverts 99ffc696d10b28580fe93441d627cf290ac4484c > > "virtio: wean net driver off NETDEV_TX_BUSY". > > > > The complexity of queuing an skb (setting a tasklet to re-xmit) is > > questionable, especially once we get rid of the other reason for the > > tasklet in the next patch. > > > > If the skb won't fit in the tx queue, just return NETDEV_TX_BUSY. It > > might be frowned upon, but it's common and not going away any time > > soon. > > This looks like a step backwards to me. If we have to do this > to fix a bug, sure. But just doing it for the sake of it smells > wrong. I disagree. We've introduced a third queue, inside the driver, one element long. That feels terribly, terribly hacky and wrong. What do we do if it overflows? Discard the packet, even if we have room in the tx queue. And when do we flush this queue? Well, that's a bit messy. Obviously we need to enable the tx interrupt when the tx queue is full, but we can't just netif_wake_queue, we have to also flush the queue. We can't do that in an irq handler, since we need to block the normal tx path (or introduce another lock and disable interrupts in our xmit routine). So we add a tasklet to do this re-transmission. Or, we could just "return NETDEV_TX_BUSY;". I like that :) Hope that clarifies, Rusty. > > Cheers, -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 02, 2009 at 11:25:57PM +0930, Rusty Russell wrote: > > Or, we could just "return NETDEV_TX_BUSY;". I like that :) No you should fix it so that you check the queue status after transmitting a packet so we never get into this state in the first place. NETDEV_TX_BUSY is just passing the problem to someone else, which is not nice at all. For example, anyone running tcpdump will now see the packet twice. Cheers,
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -47,9 +47,6 @@ struct virtnet_info struct napi_struct napi; unsigned int status; - /* The skb we couldn't send because buffers were full. */ - struct sk_buff *last_xmit_skb; - /* If we need to free in a timer, this is it. */ struct timer_list xmit_free_timer; @@ -116,9 +113,8 @@ static void skb_xmit_done(struct virtque /* We were probably waiting for more output buffers. */ netif_wake_queue(vi->dev); - /* Make sure we re-xmit last_xmit_skb: if there are no more packets - * queued, start_xmit won't be called. */ - tasklet_schedule(&vi->tasklet); + if (vi->free_in_tasklet) + tasklet_schedule(&vi->tasklet); } static void receive_skb(struct net_device *dev, struct sk_buff *skb, @@ -509,12 +505,7 @@ static void xmit_tasklet(unsigned long d struct virtnet_info *vi = (void *)data; netif_tx_lock_bh(vi->dev); - if (vi->last_xmit_skb && xmit_skb(vi, vi->last_xmit_skb) == 0) { - vi->svq->vq_ops->kick(vi->svq); - vi->last_xmit_skb = NULL; - } - if (vi->free_in_tasklet) - free_old_xmit_skbs(vi); + free_old_xmit_skbs(vi); netif_tx_unlock_bh(vi->dev); } @@ -526,27 +517,14 @@ again: /* Free up any pending old buffers before queueing new ones. */ free_old_xmit_skbs(vi); - /* If we has a buffer left over from last time, send it now. */ - if (unlikely(vi->last_xmit_skb) && - xmit_skb(vi, vi->last_xmit_skb) != 0) - goto stop_queue; + /* Put new one in send queue and do transmit */ + __skb_queue_head(&vi->send, skb); + if (likely(xmit_skb(vi, skb) == 0)) { + vi->svq->vq_ops->kick(vi->svq); + return NETDEV_TX_OK; + } - vi->last_xmit_skb = NULL; - - /* Put new one in send queue and do transmit */ - if (likely(skb)) { - __skb_queue_head(&vi->send, skb); - if (xmit_skb(vi, skb) != 0) { - vi->last_xmit_skb = skb; - skb = NULL; - goto stop_queue; - } - } -done: - vi->svq->vq_ops->kick(vi->svq); - return NETDEV_TX_OK; - -stop_queue: + /* Ring too full for this packet. */ pr_debug("%s: virtio not prepared to send\n", dev->name); netif_stop_queue(dev); @@ -557,12 +535,7 @@ stop_queue: netif_start_queue(dev); goto again; } - if (skb) { - /* Drop this skb: we only queue one. */ - vi->dev->stats.tx_dropped++; - kfree_skb(skb); - } - goto done; + return NETDEV_TX_BUSY; } static int virtnet_set_mac_address(struct net_device *dev, void *p)
This effectively reverts 99ffc696d10b28580fe93441d627cf290ac4484c "virtio: wean net driver off NETDEV_TX_BUSY". The complexity of queuing an skb (setting a tasklet to re-xmit) is questionable, especially once we get rid of the other reason for the tasklet in the next patch. If the skb won't fit in the tx queue, just return NETDEV_TX_BUSY. It might be frowned upon, but it's common and not going away any time soon. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Cc: Herbert Xu <herbert@gondor.apana.org.au> --- drivers/net/virtio_net.c | 49 ++++++++++------------------------------------- 1 file changed, 11 insertions(+), 38 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html