Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On Wed, Nov 19, 2014 at 11:46 AM, David Miller 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
From: Jason Wang
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 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
From: Jason Wang jasow...@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 jasow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On Wed, Nov 19, 2014 at 11:46 AM, David Miller da...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On 11/19/2014 03:53 AM, Cong Wang wrote:
> On Mon, Nov 17, 2014 at 9:20 PM, 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.
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
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
>> Signed-off-by: Jason Wang
>> ---
>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On Mon, Nov 17, 2014 at 9:20 PM, 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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
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
> Signed-off-by: Jason Wang
> ---
> 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.
signature.asc
Description: Digital signature
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
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 m...@redhat.com Signed-off-by: Jason Wang jasow...@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. signature.asc Description: Digital signature
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On Mon, Nov 17, 2014 at 9:20 PM, Jason Wang jasow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
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 m...@redhat.com Signed-off-by: Jason Wang jasow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On 11/19/2014 03:53 AM, Cong Wang wrote: On Mon, Nov 17, 2014 at 9:20 PM, Jason Wang jasow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
