[PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[Jason Wang]

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;
 }
 
 static void tun_net_mclist(struct net_device *dev)
-- 
1.9.1

Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

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

-- 
			Amos.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[Cong Wang]

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.

Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[Jason Wang]

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.

Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[Jason Wang]

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.

Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[David Miller]

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.

Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets

[Cong Wang]

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.