thr3ads.net - Xen devel - [Xen-devel] [PATCH] remus: fix check for installed qdiscs on ifb [Mar 2011]

If this information is useful, please help other people find it:
Share via:

Shriram Rajagopalan

2011-Mar-20 03:39 UTC

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

# 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)

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

Ian Jackson

2011-Mar-21 14:51 UTC

head link

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

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.

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

Shriram Rajagopalan

2011-Mar-21 17:56 UTC

head link

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

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 iscaused 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.
>
>

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

Shriram Rajagopalan

2011-Mar-25 19:55 UTC

head link

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

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


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

Ian Jackson

2011-Mar-31 17:07 UTC

head link

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

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.

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

Shriram Rajagopalan

2011-Mar-31 17:30 UTC

head link

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

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 becomesone 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.
>
>

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

Xen devel - Mar 2011 - [PATCH] remus: fix check for installed qdiscs on ifb

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

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

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

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

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

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