| Message ID | 1410941288-1880-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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." :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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" -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guys just change the check in macvlan to >= tx_queue_len or similar and let's end this silly discussion. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)