[PATCH] remus: fix check for installed qdiscs on ifb

[PATCH] remus: fix check for installed qdiscs on ifb

[Shriram Rajagopalan]

# HG changeset patch
# User Shriram Rajagopalan <rshriram@cs.ubc.ca>
# Date 1300592358 25200
# Node ID 87f94f4f06c4403b97c4fd4d81e3ddc29e17f88a
# Parent  a8fee4ad3ad0650e7a5cc0fb253c6a0ada1ac583
remus: fix check for installed qdiscs on ifb

current check includes ingress and pfifo_fast.
Add mq to the list of allowed qdiscs already installed
on ifb. This patch fixes cases where remus fails to start,
due to an mq qdisc already present on the vif.

Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

diff -r a8fee4ad3ad0 -r 87f94f4f06c4 tools/python/xen/remus/device.py
--- a/tools/python/xen/remus/device.py	Fri Mar 11 18:22:23 2011 +0000
+++ b/tools/python/xen/remus/device.py	Sat Mar 19 20:39:18 2011 -0700
@@ -320,9 +320,9 @@
             if q['kind'] == 'plug':
                 self.installed = True
                 return
-            if q['kind'] not in ('ingress', 'pfifo_fast'):
+            if q['kind'] not in ('ingress', 'pfifo_fast', 'mq'):
                 raise BufferedNICException('there is already a queueing '
-                                           'discipline on %s' % devname)
+                                           'discipline %s on %s' % (q['kind'], devname))
 
         print ('installing buffer on %s... ' % devname),
         req = qdisc.addrequest(self.bufdevno, self.handle, self.q)

Re: [PATCH] remus: fix check for installed qdiscs on ifb

[Ian Jackson]

Shriram Rajagopalan writes ("[Xen-devel] [PATCH] remus: fix check for installed qdiscs on ifb"):
> remus: fix check for installed qdiscs on ifb

Thanks.

> current check includes ingress and pfifo_fast.
> Add mq to the list of allowed qdiscs already installed
> on ifb. This patch fixes cases where remus fails to start,
> due to an mq qdisc already present on the vif.

Forgive me for being dense, but I don't understand this at all.  What
is the problem caused by pre-existing qdiscs that the code is trying
to avoid, and why are these particular qdiscs OK ?

Thanks,
Ian.

Re: [PATCH] remus: fix check for installed qdiscs on ifb

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 2184 bytes --]

On Mon, Mar 21, 2011 at 7:51 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>wrote:

> Shriram Rajagopalan writes ("[Xen-devel] [PATCH] remus: fix check for
> installed qdiscs on ifb"):
> > remus: fix check for installed qdiscs on ifb
>
> Thanks.
>
> > current check includes ingress and pfifo_fast.
> > Add mq to the list of allowed qdiscs already installed
> > on ifb. This patch fixes cases where remus fails to start,
> > due to an mq qdisc already present on the vif.
>
> Forgive me for being dense, but I don't understand this at all.  What
> is the problem caused by pre-existing qdiscs that the code is trying
> to avoid, and why are these particular qdiscs OK ?
>
> sorry. my bad. It is not the pre-existing qdiscs that cause an issue. It is
caused by the dummy "mq" qdisc that gets added by "default". The original
code checks for presence of only ingress/pfifo-fast qdisc. If anything else
is
present, it punts. In this case, "mq" is present (added by default) and
causes
remus to fail.

This is what I understood from the kernel netfilter code & docs.

from net/netfilter/sched/sched_generic.c
void dev_activate(struct net_device *dev)
{
        /* No queueing discipline is attached to device;
           create default one i.e. pfifo_fast for devices,
           which need queueing and noqueue_qdisc for
           virtual interfaces
         */

        if (dev->qdisc == &noop_qdisc)
                attach_default_qdiscs(dev);
....
static void attach_default_qdiscs(struct net_device *dev)
{
...
        if (!netif_is_multiqueue(dev) || dev->tx_queue_len == 0) {
                netdev_for_each_tx_queue(dev, attach_one_default_qdisc,
NULL);
       ...
        } else {
                qdisc = qdisc_create_dflt(dev, txq, &mq_qdisc_ops,
TC_H_ROOT);
       ...

sch_mq is a "Classful multiqueue dummy scheduler" and according to the
multiqueue semantics in Section 2: Documentation/networking/multiqueue.txt

"Currently two qdiscs are optimized for multiqueue devices.  The first is
the
default pfifo_fast qdisc.  This qdisc supports one qdisc per hardware queue.
A new round-robin qdisc, sch_multiq also supports multiple hardware queues."

shriram

> Thanks,
> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 2967 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] remus: fix check for installed qdiscs on ifb

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 2450 bytes --]

On Mon, Mar 21, 2011 at 10:56 AM, Shriram Rajagopalan <rshriram@cs.ubc.ca>wrote:

>
> On Mon, Mar 21, 2011 at 7:51 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>wrote:
>
>> Shriram Rajagopalan writes ("[Xen-devel] [PATCH] remus: fix check for
>> installed qdiscs on ifb"):
>> > remus: fix check for installed qdiscs on ifb
>>
>> Thanks.
>>
>> > current check includes ingress and pfifo_fast.
>> > Add mq to the list of allowed qdiscs already installed
>> > on ifb. This patch fixes cases where remus fails to start,
>> > due to an mq qdisc already present on the vif.
>>
>> Forgive me for being dense, but I don't understand this at all.  What
>> is the problem caused by pre-existing qdiscs that the code is trying
>> to avoid, and why are these particular qdiscs OK ?
>>
>> sorry. my bad. It is not the pre-existing qdiscs that cause an issue. It
> is
> caused by the dummy "mq" qdisc that gets added by "default". The original
> code checks for presence of only ingress/pfifo-fast qdisc. If anything else
> is
> present, it punts. In this case, "mq" is present (added by default) and
> causes
> remus to fail.
>
> This is what I understood from the kernel netfilter code & docs.
>
> from net/netfilter/sched/sched_generic.c
> void dev_activate(struct net_device *dev)
> {
>         /* No queueing discipline is attached to device;
>            create default one i.e. pfifo_fast for devices,
>            which need queueing and noqueue_qdisc for
>            virtual interfaces
>          */
>
>         if (dev->qdisc == &noop_qdisc)
>                 attach_default_qdiscs(dev);
> ....
> static void attach_default_qdiscs(struct net_device *dev)
> {
> ...
>         if (!netif_is_multiqueue(dev) || dev->tx_queue_len == 0) {
>                 netdev_for_each_tx_queue(dev, attach_one_default_qdisc,
> NULL);
>        ...
>         } else {
>                 qdisc = qdisc_create_dflt(dev, txq, &mq_qdisc_ops,
> TC_H_ROOT);
>        ...
>
> sch_mq is a "Classful multiqueue dummy scheduler" and according to the
> multiqueue semantics in Section 2: Documentation/networking/multiqueue.txt
>
> "Currently two qdiscs are optimized for multiqueue devices.  The first is
> the
> default pfifo_fast qdisc.  This qdisc supports one qdisc per hardware
> queue.
> A new round-robin qdisc, sch_multiq also supports multiple hardware
> queues."
>
> shriram
>
>> Thanks,
>> Ian.
>>
>>
> ping.
Ian, do you need any more justifications for this patch?

shriram

[-- Attachment #1.2: Type: text/html, Size: 3438 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] remus: fix check for installed qdiscs on ifb

[Ian Jackson]

Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH] remus: fix check for installed qdiscs on ifb"):
> sorry. my bad. It is not the pre-existing qdiscs that cause an
> issue. It is caused by the dummy "mq" qdisc that gets added by
> "default". The original code checks for presence of only
> ingress/pfifo-fast qdisc. If anything else is present, it punts. In
> this case, "mq" is present (added by default) and causes remus to
> fail.

Thanks for the explanation.  I confess I still don't really understand
why other qdiscs besides the default ones are a problem, but the
default ones aren't, but I think your patch isn't making anything
worse so I have applied it.

Thanks,
Ian.

Re: [PATCH] remus: fix check for installed qdiscs on ifb

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 1013 bytes --]

On Thu, Mar 31, 2011 at 10:07 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>wrote:

> Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH] remus: fix check for
> installed qdiscs on ifb"):
> > sorry. my bad. It is not the pre-existing qdiscs that cause an
> > issue. It is caused by the dummy "mq" qdisc that gets added by
> > "default". The original code checks for presence of only
> > ingress/pfifo-fast qdisc. If anything else is present, it punts. In
> > this case, "mq" is present (added by default) and causes remus to
> > fail.
>
> Thanks for the explanation.  I confess I still don't really understand
> why other qdiscs besides the default ones are a problem, but the
> default ones aren't, but I think your patch isn't making anything
> worse so I have applied it.
>
> So, if there is multiqueue support in the system, then mq becomes
one of the default qdiscs that could be found on an interface.
And this patch basically adds mq to the list.

Thanks for applying the patch
shriram

> Thanks,
> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 1608 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel