[net-next] tun: return NET_XMIT_DROP for dropped packets
diff mbox series

Message ID 1416288041-30921-1-git-send-email-jasowang@redhat.com
State New, archived
Headers show
Series
  • [net-next] tun: return NET_XMIT_DROP for dropped packets
Related show

Commit Message

Jason Wang Nov. 18, 2014, 5:20 a.m. UTC
After commit 5d097109257c03a71845729f8db6b5770c4bbedc
("tun: only queue packets on device"), NETDEV_TX_OK was returned for
dropped packets. This will confuse pktgen since dropped packets were
counted as sent ones.

Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
packet.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Amos Kong Nov. 18, 2014, 4:53 p.m. UTC | #1
On Tue, Nov 18, 2014 at 01:20:41PM +0800, Jason Wang wrote:
> After commit 5d097109257c03a71845729f8db6b5770c4bbedc
> ("tun: only queue packets on device"), NETDEV_TX_OK was returned for
> dropped packets. This will confuse pktgen since dropped packets were
> counted as sent ones.
> 
> Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
> packet.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e3fa65a..ac53a73 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -819,7 +819,7 @@ drop:
>  	skb_tx_error(skb);
>  	kfree_skb(skb);
>  	rcu_read_unlock();
> -	return NETDEV_TX_OK;
> +	return NET_XMIT_DROP;

Quoted from linux/drivers/firewire/net.c:

  /*
   * FIXME: According to a patch from 2003-02-26, "returning non-zero
   * causes serious problems" here, allegedly.  Before that patch,
   * -ERRNO was returned which is not appropriate under Linux 2.6.
   * Perhaps more needs to be done?  Stop the queue in serious
   * conditions and restart it elsewhere?
   */

I saw many drivers return NETDEV_TX_OK in xmit for drop packets, eg: virtio_net.c

>  }
>  
>  static void tun_net_mclist(struct net_device *dev)
> -- 
> 1.9.1
Cong Wang Nov. 18, 2014, 7:53 p.m. UTC | #2
On Mon, Nov 17, 2014 at 9:20 PM, Jason Wang <jasowang@redhat.com> wrote:
> After commit 5d097109257c03a71845729f8db6b5770c4bbedc
> ("tun: only queue packets on device"), NETDEV_TX_OK was returned for
> dropped packets. This will confuse pktgen since dropped packets were
> counted as sent ones.
>
> Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
> packet.

pktgen is suspicious, it sends out packets directly without going through
qdisc, so it should not care about NET_XMIT_* qdisc error code?

Looks like NETDEV_TX_OK doesn't have to mean TX is successful,
the comment says driver takes care of the packet, can be either dropped
or sent out. We might need a new code to distinguish success or failure.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang Nov. 19, 2014, 3:09 a.m. UTC | #3
On 11/19/2014 12:53 AM, Amos Kong wrote:
> On Tue, Nov 18, 2014 at 01:20:41PM +0800, Jason Wang wrote:
>> After commit 5d097109257c03a71845729f8db6b5770c4bbedc
>> ("tun: only queue packets on device"), NETDEV_TX_OK was returned for
>> dropped packets. This will confuse pktgen since dropped packets were
>> counted as sent ones.
>>
>> Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
>> packet.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  drivers/net/tun.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index e3fa65a..ac53a73 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -819,7 +819,7 @@ drop:
>>  	skb_tx_error(skb);
>>  	kfree_skb(skb);
>>  	rcu_read_unlock();
>> -	return NETDEV_TX_OK;
>> +	return NET_XMIT_DROP;
> Quoted from linux/drivers/firewire/net.c:
>
>   /*
>    * FIXME: According to a patch from 2003-02-26, "returning non-zero
>    * causes serious problems" here, allegedly.  Before that patch,
>    * -ERRNO was returned which is not appropriate under Linux 2.6.
>    * Perhaps more needs to be done?  Stop the queue in serious
>    * conditions and restart it elsewhere?
>    */
>
> I saw many drivers return NETDEV_TX_OK in xmit for drop packets, eg: virtio_net.c

Well, I think you miss some thing:

- Virtio-net only drop packets when there's a bug in either driver or
hypervisor. Other drivers only drop the bad packets. For pktgen, we can
make sure the packet is good.
- Most of the drivers (included virtio-net but not tun) will stop txq
before the ring is full, this could be detected by pktgen
- Tun keep accepting packets and dropping them even if the socket
receive queue is full.

So we really need NET_XMIT_DROP here to let pktgen know the packets were
not sent correctly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang Nov. 19, 2014, 3:15 a.m. UTC | #4
On 11/19/2014 03:53 AM, Cong Wang wrote:
> On Mon, Nov 17, 2014 at 9:20 PM, Jason Wang <jasowang@redhat.com> wrote:
>> > After commit 5d097109257c03a71845729f8db6b5770c4bbedc
>> > ("tun: only queue packets on device"), NETDEV_TX_OK was returned for
>> > dropped packets. This will confuse pktgen since dropped packets were
>> > counted as sent ones.
>> >
>> > Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
>> > packet.
> pktgen is suspicious, it sends out packets directly without going through
> qdisc, so it should not care about NET_XMIT_* qdisc error code?

Well, NET_XMIT_DROP has been used by some devices. I don't see any side
effect of using this especially consider that pktgen can recognize them.
> Looks like NETDEV_TX_OK doesn't have to mean TX is successful,
> the comment says driver takes care of the packet, can be either dropped
> or sent out. We might need a new code to distinguish success or failure.

Most drivers only drop bad packets when they return NETDEV_TX_OK and
they will stop the txq before tx ring is full. This is not the case of
tun, it never stop txq and keep accepting packets and dropping them when
socket receive queue is full.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
David Miller Nov. 19, 2014, 7:46 p.m. UTC | #5
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 19 Nov 2014 11:15:36 +0800

> On 11/19/2014 03:53 AM, Cong Wang wrote:
>> On Mon, Nov 17, 2014 at 9:20 PM, Jason Wang <jasowang@redhat.com> wrote:
>>> > After commit 5d097109257c03a71845729f8db6b5770c4bbedc
>>> > ("tun: only queue packets on device"), NETDEV_TX_OK was returned for
>>> > dropped packets. This will confuse pktgen since dropped packets were
>>> > counted as sent ones.
>>> >
>>> > Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
>>> > packet.
>> pktgen is suspicious, it sends out packets directly without going through
>> qdisc, so it should not care about NET_XMIT_* qdisc error code?
> 
> Well, NET_XMIT_DROP has been used by some devices. I don't see any side
> effect of using this especially consider that pktgen can recognize them.
>> Looks like NETDEV_TX_OK doesn't have to mean TX is successful,
>> the comment says driver takes care of the packet, can be either dropped
>> or sent out. We might need a new code to distinguish success or failure.
> 
> Most drivers only drop bad packets when they return NETDEV_TX_OK and
> they will stop the txq before tx ring is full. This is not the case of
> tun, it never stop txq and keep accepting packets and dropping them when
> socket receive queue is full.

Agreed.

Often the issue with TX return values is lack of clear documentation
and use cases.

I've applied this patch, thanks Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Cong Wang Nov. 19, 2014, 7:53 p.m. UTC | #6
On Wed, Nov 19, 2014 at 11:46 AM, David Miller <davem@davemloft.net> wrote:
>
> Often the issue with TX return values is lack of clear documentation
> and use cases.
>

NET_XMIT_* are marked as qdisc ->enqueue() return codes
in include/linux/netdevice.h, and are indeed used mostly by
qdisc:

$ git grep NET_XMIT_ -- net/sched/ | wc -l
88
$ git grep NET_XMIT_ -- drivers/net/ | wc -l
15

It is a mess. Not to mention we have __NET_XMIT_ code too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e3fa65a..ac53a73 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -819,7 +819,7 @@  drop:
 	skb_tx_error(skb);
 	kfree_skb(skb);
 	rcu_read_unlock();
-	return NETDEV_TX_OK;
+	return NET_XMIT_DROP;
 }
 
 static void tun_net_mclist(struct net_device *dev)