* [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
@ 2014-09-17 8:08 Nicolas Dichtel
2014-09-19 20:03 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Nicolas Dichtel @ 2014-09-17 8:08 UTC (permalink / raw)
To: davem; +Cc: netdev, Nicolas Dichtel, Herbert Xu
Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
driver uses tx_queue_len of the master device as the limit of packets enqueuing.
Problem is that virtual drivers have this value set to 0, thus all broadcast
packets were rejected.
Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
of 1000 (also arbitrarily chosen).
CC: Herbert Xu <herbert@gondor.apana.org.au>
Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
Suggested-by: Thibaut Collet <thibaut.collet@6wind.com>
Tested-by: Thibaut Collet <thibaut.collet@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
drivers/net/macvlan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index a96955597755..8e776935ca52 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -36,6 +36,7 @@
#include <linux/netpoll.h>
#define MACVLAN_HASH_SIZE (1 << BITS_PER_BYTE)
+#define MACVLAN_BC_QUEUE_LEN 1000
struct macvlan_port {
struct net_device *dev;
@@ -248,7 +249,7 @@ static void macvlan_broadcast_enqueue(struct macvlan_port *port,
goto err;
spin_lock(&port->bc_queue.lock);
- if (skb_queue_len(&port->bc_queue) < skb->dev->tx_queue_len) {
+ if (skb_queue_len(&port->bc_queue) < MACVLAN_BC_QUEUE_LEN) {
__skb_queue_tail(&port->bc_queue, nskb);
err = 0;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-17 8:08 [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device Nicolas Dichtel
@ 2014-09-19 20:03 ` David Miller
2014-09-19 23:31 ` Herbert Xu
2014-09-19 21:24 ` Cong Wang
2014-09-22 18:10 ` David Miller
2 siblings, 1 reply; 14+ messages in thread
From: David Miller @ 2014-09-19 20:03 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, herbert
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 17 Sep 2014 10:08:08 +0200
> Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
> driver uses tx_queue_len of the master device as the limit of packets enqueuing.
> Problem is that virtual drivers have this value set to 0, thus all broadcast
> packets were rejected.
> Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
> of 1000 (also arbitrarily chosen).
>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
> Suggested-by: Thibaut Collet <thibaut.collet@6wind.com>
> Tested-by: Thibaut Collet <thibaut.collet@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Herbert please review.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-17 8:08 [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device Nicolas Dichtel
2014-09-19 20:03 ` David Miller
@ 2014-09-19 21:24 ` Cong Wang
2014-09-19 21:35 ` Eric Dumazet
2014-09-22 18:10 ` David Miller
2 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-09-19 21:24 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: David Miller, netdev, Herbert Xu
On Wed, Sep 17, 2014 at 1:08 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
> driver uses tx_queue_len of the master device as the limit of packets enqueuing.
> Problem is that virtual drivers have this value set to 0, thus all broadcast
> packets were rejected.
> Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
> of 1000 (also arbitrarily chosen).
>
Hmm, why not manually set this tx_queue_len to fix it?
At least fifo qdisc also uses tx_queue_len, and when you bond
you have to set it to non-zero.
I am not saying using this tx_queue_len is absolutely correct,
I am saying it may be expected to set tx_queue_len by yourself.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 21:24 ` Cong Wang
@ 2014-09-19 21:35 ` Eric Dumazet
2014-09-19 21:59 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2014-09-19 21:35 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, 2014-09-19 at 14:24 -0700, Cong Wang wrote:
> On Wed, Sep 17, 2014 at 1:08 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
> > Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
> > driver uses tx_queue_len of the master device as the limit of packets enqueuing.
> > Problem is that virtual drivers have this value set to 0, thus all broadcast
> > packets were rejected.
> > Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
> > of 1000 (also arbitrarily chosen).
> >
>
> Hmm, why not manually set this tx_queue_len to fix it?
>
> At least fifo qdisc also uses tx_queue_len, and when you bond
> you have to set it to non-zero.
This is not true. We usually leave virtual devices with tx_queue_len to
0, otherwise you get a qdisc on them for nothing...
Only when you want to have say HTB on the bonding, you might need to
change tx_queue_len
>
> I am not saying using this tx_queue_len is absolutely correct,
> I am saying it may be expected to set tx_queue_len by yourself.
I disagree.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 21:35 ` Eric Dumazet
@ 2014-09-19 21:59 ` Cong Wang
2014-09-19 22:05 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-09-19 21:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, Sep 19, 2014 at 2:35 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-09-19 at 14:24 -0700, Cong Wang wrote:
>> On Wed, Sep 17, 2014 at 1:08 AM, Nicolas Dichtel
>> <nicolas.dichtel@6wind.com> wrote:
>> > Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
>> > driver uses tx_queue_len of the master device as the limit of packets enqueuing.
>> > Problem is that virtual drivers have this value set to 0, thus all broadcast
>> > packets were rejected.
>> > Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
>> > of 1000 (also arbitrarily chosen).
>> >
>>
>> Hmm, why not manually set this tx_queue_len to fix it?
>>
>> At least fifo qdisc also uses tx_queue_len, and when you bond
>> you have to set it to non-zero.
>
>
> This is not true. We usually leave virtual devices with tx_queue_len to
> 0, otherwise you get a qdisc on them for nothing...
Yeah, actually it is 1 if tx_queue_len is 0, my memory mistaken.
But many virtual devices like veth call ether_setup() therefore have
tx_queue_len = 1000, not 0. dummy and bonding are 0 though.
>
> Only when you want to have say HTB on the bonding, you might need to
> change tx_queue_len
HTB by default creates a fifo qdisc.... same thing as above.
>
>>
>> I am not saying using this tx_queue_len is absolutely correct,
>> I am saying it may be expected to set tx_queue_len by yourself.
>
> I disagree.
>
Then fix these tx_queue_len = 0?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 21:59 ` Cong Wang
@ 2014-09-19 22:05 ` Eric Dumazet
2014-09-19 22:14 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2014-09-19 22:05 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, 2014-09-19 at 14:59 -0700, Cong Wang wrote:
>
> Then fix these tx_queue_len = 0?
What are you suggesting exactly ?
We want them being 0, exactly.
This is the standard way many scripts are able to remove a qdisc.
We dont want to break user scripts. Ever.
tc qdisc add dev bond0 root sfq
...
ifconfig bond0 txqueuelen 0
tc qdisc del dev bond0 root
-> No qdisc
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 22:05 ` Eric Dumazet
@ 2014-09-19 22:14 ` Cong Wang
2014-09-19 22:44 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-09-19 22:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, Sep 19, 2014 at 3:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-09-19 at 14:59 -0700, Cong Wang wrote:
>
>>
>> Then fix these tx_queue_len = 0?
>
> What are you suggesting exactly ?
Quote:
">
>>
>> I am not saying using this tx_queue_len is absolutely correct,
>> I am saying it may be expected to set tx_queue_len by yourself.
>
> I disagree.
>"
So you disagree with setting tx_queue_len manually, then it means
you think every tx_queue_len should have a non-zero value, therefore
those tx_queue_len = 0 should be fixed?
>
> We want them being 0, exactly.
Conflicts with what you claimed by "I disagree." :)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 22:14 ` Cong Wang
@ 2014-09-19 22:44 ` Eric Dumazet
2014-09-19 22:57 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2014-09-19 22:44 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, 2014-09-19 at 15:14 -0700, Cong Wang wrote:
> Conflicts with what you claimed by "I disagree." :)
Whats wrong with using macvlan on top of bonding device where
tx_queue_len needs to be 0, because we do not want a qdisc on it ?
Therefore, suggesting user to 'fix tx_queue_len' is totally wrong.
Fix macvlan driver, and do not try to 'fix user scripts'
Thats very simple.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 22:44 ` Eric Dumazet
@ 2014-09-19 22:57 ` Cong Wang
2014-09-19 23:09 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-09-19 22:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, Sep 19, 2014 at 3:44 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Therefore, suggesting user to 'fix tx_queue_len' is totally wrong.
How about HTB? You said:
"Only when you want to have say HTB on the bonding, you might need to
change tx_queue_len"
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 22:57 ` Cong Wang
@ 2014-09-19 23:09 ` Eric Dumazet
2014-09-20 2:00 ` David Miller
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2014-09-19 23:09 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, David Miller, netdev, Herbert Xu
On Fri, 2014-09-19 at 15:57 -0700, Cong Wang wrote:
> On Fri, Sep 19, 2014 at 3:44 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Therefore, suggesting user to 'fix tx_queue_len' is totally wrong.
>
> How about HTB? You said:
>
> "Only when you want to have say HTB on the bonding, you might need to
> change tx_queue_len"
Again, what is your point ?
How is it relevant to this particular bug report ?
Don't you think people using HTB on a virtual device do not already know
its better to either :
- Not use the default pfifo on htb classes
- Increase device queue len on the device.
Its been like that since a decade. No changes are needed in bond driver.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 20:03 ` David Miller
@ 2014-09-19 23:31 ` Herbert Xu
2014-09-19 23:44 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Herbert Xu @ 2014-09-19 23:31 UTC (permalink / raw)
To: David Miller; +Cc: nicolas.dichtel, netdev
On Fri, Sep 19, 2014 at 04:03:51PM -0400, David Miller wrote:
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Wed, 17 Sep 2014 10:08:08 +0200
>
> > Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
> > driver uses tx_queue_len of the master device as the limit of packets enqueuing.
> > Problem is that virtual drivers have this value set to 0, thus all broadcast
> > packets were rejected.
> > Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
> > of 1000 (also arbitrarily chosen).
> >
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
> > Suggested-by: Thibaut Collet <thibaut.collet@6wind.com>
> > Tested-by: Thibaut Collet <thibaut.collet@6wind.com>
> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> Herbert please review.
While it would be nice to maintain the ability to chnge this
parameter, I'm fine with this patch as it stands since it fixes
a real regression.
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 23:31 ` Herbert Xu
@ 2014-09-19 23:44 ` Eric Dumazet
0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2014-09-19 23:44 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, nicolas.dichtel, netdev
On Sat, 2014-09-20 at 07:31 +0800, Herbert Xu wrote:
> While it would be nice to maintain the ability to chnge this
> parameter, I'm fine with this patch as it stands since it fixes
> a real regression.
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Another interesting feature would be to be able to time limit the queue,
and/or use a head drop instead of tail drop.
(ie timestamp skb when its enqueued into bc_queue,
and at dequeue time, drop it if elapsed time is above a configurable
limit)
Processing a broadcast one minute after it was really sent makes little
sense, especially if we drop new incoming broadcasts because bc_queue is
full.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-19 23:09 ` Eric Dumazet
@ 2014-09-20 2:00 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2014-09-20 2:00 UTC (permalink / raw)
To: eric.dumazet; +Cc: cwang, nicolas.dichtel, netdev, herbert
Guys just change the check in macvlan to >= tx_queue_len or similar
and let's end this silly discussion.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
2014-09-17 8:08 [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device Nicolas Dichtel
2014-09-19 20:03 ` David Miller
2014-09-19 21:24 ` Cong Wang
@ 2014-09-22 18:10 ` David Miller
2 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2014-09-22 18:10 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, herbert
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 17 Sep 2014 10:08:08 +0200
> Since commit 412ca1550cbe ("macvlan: Move broadcasts into a work queue"), the
> driver uses tx_queue_len of the master device as the limit of packets enqueuing.
> Problem is that virtual drivers have this value set to 0, thus all broadcast
> packets were rejected.
> Because tx_queue_len was arbitrarily chosen, I replace it with a static limit
> of 1000 (also arbitrarily chosen).
>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
> Suggested-by: Thibaut Collet <thibaut.collet@6wind.com>
> Tested-by: Thibaut Collet <thibaut.collet@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Applied and queued up for -stable.
Longer term we should come up with something a little bit nicer.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-09-22 18:10 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-17 8:08 [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device Nicolas Dichtel
2014-09-19 20:03 ` David Miller
2014-09-19 23:31 ` Herbert Xu
2014-09-19 23:44 ` Eric Dumazet
2014-09-19 21:24 ` Cong Wang
2014-09-19 21:35 ` Eric Dumazet
2014-09-19 21:59 ` Cong Wang
2014-09-19 22:05 ` Eric Dumazet
2014-09-19 22:14 ` Cong Wang
2014-09-19 22:44 ` Eric Dumazet
2014-09-19 22:57 ` Cong Wang
2014-09-19 23:09 ` Eric Dumazet
2014-09-20 2:00 ` David Miller
2014-09-22 18:10 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).