Stephen Hemminger
2015-Mar-24 23:22 UTC
[PATCH net-next] virtio: change comment in transmit
The original comment was not really informative or funny
as well as sexist. Replace it with a better explanation of
why the driver does stop 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_BUSY (and cause a BUG message).
+ * Since most packets only take 1 or 2 ring slots
+ * this means 16 slots are typically wasted.
+ */
if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
netif_stop_subqueue(dev, qnum);
if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
From: Stephen Hemminger <stephen at networkplumber.org> Date: Tue, 24 Mar 2015 16:22:07 -0700> The original comment was not really informative or funny > as well as sexist. Replace it with a better explanation of > why the driver does stop and what the impacts are. > > Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>Applied, but _only_ because it adds clarity. I absolutely do not believe in this for the sake of making people easily made upset feel better, that's censorship and it's crap.
Stephen Hemminger <stephen at networkplumber.org> writes:> The original comment was not really informative or funny > as well as sexist. Replace it with a better explanation of > why the driver does stop and what the impacts are. > > Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>Fair call. Comment was certainly snarky, probably sexist. I think it expressed my feelings perfectly, however. I note that there's still no comment saying "don't do this" in netdevice.h; I gather returning NETDEV_TX_BUSY is still considered a Bad Thing? (Does it really BUG_ON?) Thanks, Rusty.> --- 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_BUSY (and cause a BUG message). > + * Since most packets only take 1 or 2 ring slots > + * this means 16 slots are typically wasted. > + */ > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > netif_stop_subqueue(dev, qnum); > if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Rusty Russell <rusty at rustcorp.com.au> Date: Wed, 25 Mar 2015 14:29:38 +1030> I note that there's still no comment saying "don't do this" in > netdevice.h; I gather returning NETDEV_TX_BUSY is still considered a Bad > Thing?You're not supposed to do it still, that's right. If the driver returns NETDEV_TX_BUSY it means that it is not maintaining the TX queues stop/start state properly. It was kept around for drivers that we didn't want to convert or fix up at the time (this is circa two decades ago). I realize that there are situations for some devices where this is difficult to achieve, but the stack really does a non-trivial amount of useless work if you don't play along nicely. If you grep drivers/net for it, the majority of the returns are in assertion failure paths in the TX handler of those drivers. NETDEV_TX_BUSY is not being returned in a normal path of operation.> (Does it really BUG_ON?)No, it doesn't.