* Re: [PATCH] net-sched: sch_cbq: avoid infinite loop
From: David Miller @ 2012-09-12 2:21 UTC (permalink / raw)
To: eric.dumazet; +Cc: denys, netdev
In-Reply-To: <1347405072.13103.675.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 12 Sep 2012 01:11:12 +0200
> From: Eric Dumazet <edumazet@google.com>
>
> Its possible to setup a bad cbq configuration leading to
> an infinite loop in cbq_classify()
>
> DEV_OUT=eth0
> ICMP="match ip protocol 1 0xff"
> U32="protocol ip u32"
> DST="match ip dst"
> tc qdisc add dev $DEV_OUT root handle 1: cbq avpkt 1000 \
> bandwidth 100mbit
> tc class add dev $DEV_OUT parent 1: classid 1:1 cbq \
> rate 512kbit allot 1500 prio 5 bounded isolated
> tc filter add dev $DEV_OUT parent 1: prio 3 $U32 \
> $ICMP $DST 192.168.3.234 flowid 1:
>
> Reported-by: Denys Fedoryschenko <denys@visp.net.lb>
> Tested-by: Denys Fedoryschenko <denys@visp.net.lb>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply
* RE: [PATCH] checkpatch: Check networking specific block comment style
From: Allan, Bruce W @ 2012-09-12 1:19 UTC (permalink / raw)
To: Joe Perches, Andrew Morton; +Cc: Andy Whitcroft, David Miller, LKML, netdev
In-Reply-To: <1347410853.2456.7.camel@joe2Laptop>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Joe Perches
> Sent: Tuesday, September 11, 2012 5:48 PM
> To: Andrew Morton
> Cc: Andy Whitcroft; David Miller; LKML; netdev
> Subject: [PATCH] checkpatch: Check networking specific block comment
> style
>
> In an effort to get fewer checkpatch reviewer corrections,
> add a networking specific style test for the preferred
> networking comment style.
>
> /* The preferred style for block comments in
> * drivers/net/... and net/... is like this
> */
>
> These tests are only used in net/ and drivers/net/
>
> Tested with:
>
> $ cat drivers/net/t.c
>
> /* foo */
>
> /*
> * foo
> */
>
> /* foo
> */
>
> /* foo
> * bar */
> $ ./scripts/checkpatch.pl -f drivers/net/t.c
> WARNING: networking block comments don't use an empty /* line, use /* Comment...
This conflicts with the preferred style for long (multi-line) comments documented in
./Documentation/CodingStyle. If this is the way comments should be done in the
networking code this patch should also include an update to Chapter 8 in CodingStyle
documenting the networking specific style to avoid confusion.
^ permalink raw reply
* [PATCH] checkpatch: Check networking specific block comment style
From: Joe Perches @ 2012-09-12 0:47 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andy Whitcroft, David Miller, LKML, netdev
In an effort to get fewer checkpatch reviewer corrections,
add a networking specific style test for the preferred
networking comment style.
/* The preferred style for block comments in
* drivers/net/... and net/... is like this
*/
These tests are only used in net/ and drivers/net/
Tested with:
$ cat drivers/net/t.c
/* foo */
/*
* foo
*/
/* foo
*/
/* foo
* bar */
$ ./scripts/checkpatch.pl -f drivers/net/t.c
WARNING: networking block comments don't use an empty /* line, use /* Comment...
#4: FILE: net/t.c:4:
+
+/*
WARNING: networking block comments put the trailing */ on a separate line
#12: FILE: net/t.c:12:
+ * bar */
total: 0 errors, 2 warnings, 12 lines checked
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ca05ba2..7165516 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1873,6 +1873,20 @@ sub process {
"No space is necessary after a cast\n" . $hereprev);
}
+ if ($realfile =~ m@^(drivers/net/|net/)@ &&
+ $rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
+ $prevrawline =~ /^\+[ \t]*$/) {
+ WARN("NETWORKING_BLOCK_COMMENT_STYLE",
+ "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
+ }
+
+ if ($realfile =~ m@^(drivers/net/|net/)@ &&
+ $rawline !~ m@^\+[ \t]*(\/\*|\*\/)@ &&
+ $rawline =~ m@^\+[ \t]*.+\*\/[ \t]*$@) {
+ WARN("NETWORKING_BLOCK_COMMENT_STYLE",
+ "networking block comments put the trailing */ on a separate line\n" . $herecurr);
+ }
+
# check for spaces at the beginning of a line.
# Exceptions:
# 1) within comments
^ permalink raw reply related
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Rusty Russell @ 2012-09-12 0:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, rick.jones2, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <20120910061629.GC16819@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> In other words RPS is a hack to speed up networking on cheapo
> hardware, this is one of the reasons it is off by default.
> Good hardware has multiple receive queues.
> We can implement a good one so we do not need RPS.
>
> Also not all guest OS-es support RPS.
>
> Does this clarify?
Ok, thanks.
BTW, I found a better description by Tom Herbert, BTW:
https://code.google.com/p/kernel/wiki/NetScalingGuide
Now, I find the description of VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX
confusing:
1) AFAICT it turns on multiqueue rx, with no semantics attached.
I have no idea why it's called what it is. Why?
2) We've said we can remove steering methods, but we haven't actually
defined any, as we've left it completely open.
If I were a driver author, it leaves me completely baffled on how to
implement the spec :(
What are we actually planning to implement at the moment?
>For best performance, packets from a single connection should utilize
>the paired transmit and receive queues from the same virtqueue pair;
>for example both transmitqN and receiveqN. This rule makes it possible
>to optimize processing on the device side, but this is not a hard
>requirement: devices should function correctly even when this rule is
>not followed.
Why is this true? I don't actually see why the queues are in pairs at
all; are tx and rx not completely independent? So why does it matter?
>> When the steering rule is modified, some packets can still be
>> outstanding in one or more of the virtqueues. Device is not required
>> to wait for these packets to be consumed before delivering packets
>> using the new streering rule. Drivers modifying the steering rule at
>> a high rate (e.g. adaptively in response to changes in the workload)
>> are recommended to complete processing of the receive queue(s)
>> utilized by the original steering before processing any packets
>> delivered by the modified steering rule.
How can this be done? This isn't actually possible without taking the
queue down, since more packets are incoming.
Cheers,
Rusty.
^ permalink raw reply
* [PATCH] net_tx_action: Call trace_consume_skb() instead of trace_kfree_skb()
From: Shawn Bohrer @ 2012-09-11 23:28 UTC (permalink / raw)
To: netdev; +Cc: sanagi.koki, davem, eric.dumazet, Shawn Bohrer
Call trace_consume_skb() instead of trace_kfree_skb() as skbs are
removed from the completion_queue during transmit. This avoids false
positives from dropwatch/drop_monitor making them more useful.
Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
In my case I seem to hit this tracepoint for every packet I transmit so
these appear to be false positives to me. Perhaps there are cases where
you could hit this and it is a real packet drop?
net/core/dev.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 8398836..00774ce 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3015,7 +3015,7 @@ static void net_tx_action(struct softirq_action *h)
clist = clist->next;
WARN_ON(atomic_read(&skb->users));
- trace_kfree_skb(skb, net_tx_action);
+ trace_consume_skb(skb);
__kfree_skb(skb);
}
}
--
1.7.7.6
--
---------------------------------------------------------------
This email, along with any attachments, is confidential. If you
believe you received this message in error, please contact the
sender immediately and delete all copies of the message.
Thank you.
^ permalink raw reply related
* [PATCH] net-sched: sch_cbq: avoid infinite loop
From: Eric Dumazet @ 2012-09-11 23:11 UTC (permalink / raw)
To: Denys Fedoryshchenko, David Miller; +Cc: netdev
In-Reply-To: <ceeee25e4588b250ce7b363afe97a489@visp.net.lb>
From: Eric Dumazet <edumazet@google.com>
Its possible to setup a bad cbq configuration leading to
an infinite loop in cbq_classify()
DEV_OUT=eth0
ICMP="match ip protocol 1 0xff"
U32="protocol ip u32"
DST="match ip dst"
tc qdisc add dev $DEV_OUT root handle 1: cbq avpkt 1000 \
bandwidth 100mbit
tc class add dev $DEV_OUT parent 1: classid 1:1 cbq \
rate 512kbit allot 1500 prio 5 bounded isolated
tc filter add dev $DEV_OUT parent 1: prio 3 $U32 \
$ICMP $DST 192.168.3.234 flowid 1:
Reported-by: Denys Fedoryschenko <denys@visp.net.lb>
Tested-by: Denys Fedoryschenko <denys@visp.net.lb>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/sched/sch_cbq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 6aabd77..564b9fc 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -250,10 +250,11 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
else if ((cl = defmap[res.classid & TC_PRIO_MAX]) == NULL)
cl = defmap[TC_PRIO_BESTEFFORT];
- if (cl == NULL || cl->level >= head->level)
+ if (cl == NULL)
goto fallback;
}
-
+ if (cl->level >= head->level)
+ goto fallback;
#ifdef CONFIG_NET_CLS_ACT
switch (result) {
case TC_ACT_QUEUED:
^ permalink raw reply related
* Re: CBQ(but probably u32 filter bug), kernel "freeze", at least from 3.2.0
From: Denys Fedoryshchenko @ 2012-09-11 22:50 UTC (permalink / raw)
To: netdev, Eric Dumazet
>No problem, I reproduced the bug on my dev machine, but its always
>better to have bug reporter adding its own 'Tested-by:' tag ;)
>
>Thanks
Tested-by: Denys Fedoryschenko <denys@visp.net.lb>
Thank you a lot, it fixes the problem!
---
Denys Fedoryshchenko, Network Engineer, Virtual ISP S.A.L.
^ permalink raw reply
* Re: [PATCH] scsi_netlink: Remove dead and buggy code
From: David Miller @ 2012-09-11 22:48 UTC (permalink / raw)
To: ebiederm; +Cc: James.Bottomley, netdev, James.Smart, linux-scsi
In-Reply-To: <878vcggp4n.fsf@xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 11 Sep 2012 15:40:08 -0700
> So just for curiosity I searched the entire git history for scsi_nl_add_
> and the only commit that I found was the addition of that code to the
> tree in August of 2008.
>
> Does anyone have any reason to keep scsi_nl_add_transport or
> scsi_nl_add_driver that have never been used in the 4 years since
> they have been added?
That's basically the question on the table right now :-)
^ permalink raw reply
* Re: [PATCH] scsi_netlink: Remove dead and buggy code
From: Eric W. Biederman @ 2012-09-11 22:40 UTC (permalink / raw)
To: James Bottomley; +Cc: David Miller, netdev, James.Smart, linux-scsi
In-Reply-To: <1347314879.3038.74.camel@dabdike.int.hansenpartnership.com>
James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> On Mon, 2012-09-10 at 15:07 -0400, David Miller wrote:
>> From: ebiederm@xmission.com (Eric W. Biederman)
>> Date: Fri, 07 Sep 2012 15:39:21 -0700
>>
>> >
>> > The scsi netlink code confuses the netlink port id with a process id,
>> > going so far as to read NETLINK_CREDS(skb)->pid instead of the correct
>> > NETLINK_CB(skb).pid. Fortunately it does not matter because nothing
>> > registers to respond to scsi netlink requests.
>> >
>> > The only interesting use of the scsi_netlink interface is
>> > fc_host_post_vendor_event which sends a netlink multicast message.
>> >
>> > Since nothing registers to handle scsi netlink messages kill all of the
>> > registration logic, while retaining the same error handling behavior
>> > preserving the userspace visible behavior and removing all of the
>> > confused code that thought a netlink port id was a process id.
>> >
>> > This was tested with a kernel allyesconfig build which had no problems.
>> >
>> > Cc: James Bottomley <James.Bottomley@parallels.com>
>> > Cc: James Smart <James.Smart@Emulex.Com>
>> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> James et al., please review and ACK.
>
> I'll defer to the decision of James Smart and the other FC contributors,
> since the FC transport class is really the only one using a netlink
> interface.
So just for curiosity I searched the entire git history for scsi_nl_add_
and the only commit that I found was the addition of that code to the
tree in August of 2008.
Does anyone have any reason to keep scsi_nl_add_transport or
scsi_nl_add_driver that have never been used in the 4 years since
they have been added?
Eric
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-09-11 21:39 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120911213646.GF7677-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Hi Tejun,
On 09/11/2012 11:36 PM, Tejun Heo wrote:
> On Tue, Sep 11, 2012 at 11:31:45PM +0200, Daniel Wagner wrote:
>>> Oops, that was wrong. net_prio_subsys_id itself becomes constant.
>>> Let's please better explain why the RCU trick removal is safe then.
>>
>> In the last paragraph in the commit message I tried to document why
>> it is safe to remove the RCU trick. Not good enough?
>
> It isn't clear to me why it was necessary before and why it now
> becomes unnecessary. It states what the code does and that it's no
> longer necessary but I'd really like more elaboration.
Got it. I'll try to document then why it was necessary before. I'll add
then also the original author of those lines to the Cc list just in case
I get it wrong.
cheers,
daniel
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-09-11 21:36 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <504FADC1.4060503-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
Hello, Daniel.
On Tue, Sep 11, 2012 at 11:31:45PM +0200, Daniel Wagner wrote:
> >Oops, that was wrong. net_prio_subsys_id itself becomes constant.
> >Let's please better explain why the RCU trick removal is safe then.
>
> In the last paragraph in the commit message I tried to document why
> it is safe to remove the RCU trick. Not good enough?
It isn't clear to me why it was necessary before and why it now
becomes unnecessary. It states what the code does and that it's no
longer necessary but I'd really like more elaboration.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-09-11 21:31 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev, cgroups, Daniel Wagner, David S. Miller, Andrew Morton,
Eric Dumazet, Gao feng, Glauber Costa, Jamal Hadi Salim,
John Fastabend, Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120911210821.GB7677@google.com>
On 09/11/2012 11:08 PM, Tejun Heo wrote:
> On Tue, Sep 11, 2012 at 02:01:09PM -0700, Tejun Heo wrote:
>> For example, it's not evident the above is correct and it's burried
>> with all other changes. Can't we just assign the fixed subsys ID to
>> net_prio_subsys_id in this patch? This patch would be correct without
>> any netprio changes, no?
>
> Oops, that was wrong. net_prio_subsys_id itself becomes constant.
> Let's please better explain why the RCU trick removal is safe then.
In the last paragraph in the commit message I tried to document why it
is safe to remove the RCU trick. Not good enough?
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-09-11 21:27 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev, cgroups, Daniel Wagner, David S. Miller, Andrew Morton,
Eric Dumazet, Gao feng, Glauber Costa, Jamal Hadi Salim,
John Fastabend, Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <504FA9D6.4050201@monom.org>
Hello, Daniel
On Tue, Sep 11, 2012 at 11:15:02PM +0200, Daniel Wagner wrote:
> If net_prio_subsys_id is changed to be an enum, then the compiler
> will report an error:
>
> error: lvalue required as left operand of assignment
>
> that was the reason why I kept this change here. I think I just
> don't get what you are trying to tell me.
>
> >Please separate these changes and explain them.
>
> I will do that as soon I figured out what you are telling me.
Sorry about that. I was thinking that was a separate variable. Well,
we can introduce a variable, change the id allocation and then swap it
back to the constant, but that would be too much. Let's just try to
explain it better.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 0/8] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-09-11 21:19 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120911211151.GC7677-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Hi Tejun,
On 09/11/2012 11:11 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, Sep 11, 2012 at 06:26:06PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>
>> Hi,
>>
>> In this version I tried to concentrate on the main topic of this
>> series, so I removed some of the things which were not really needed
>> and I have to admit the result looks much better. So I hope that will
>> simplify the review for you.
>
> Other than the few nits I like the whole series and, given that all
> the changes are entangled with cgroup core, I would like to route it
> through cgroup/for-3.7 if nobody objects. Daniel, can you please
> rebase on top of the following branch when sending out updated
> version?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.7
Sure, I'll rebase and update the series tomorrow. Thanks for your
patience with me.
cheers,
daniel
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-09-11 21:15 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120911210435.GA7677-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Hi Tejun,
On 09/11/2012 11:04 PM, Tejun Heo wrote:
> Hello, Daniel.
>
> One more thing.
>
> On Tue, Sep 11, 2012 at 06:26:13PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>
>> WARNING: With this change it is not possible to load external built
>> controllers anymore.
>>
>> In case where CONFIG_NETPRIO_CGROUP=m and CONFIG_NET_CLS_CGROUP=m is
>> set, the type of the corresponding subsys_id should also be of type
>> enum. Up to now, net_prio_subsys_id and net_cls_subsys_id would be an
>> int in this configuration.
>>
>> With switching the macro definition IS_SUBSYS_ENABLED from IS_BUILTIN
>> to IS_ENABLED, the subsys_id will always be enum for all
>> subsystems. That means we need to remove all the code which assumes
>> that net_prio_subsys_id and net_cls_subsys_id is of type int.
>
> I don't think int or enum is the matter here. enum is an int. It's
> whether the ID is allocated statically or dynamically. Can you please
> update the description using those terms instead?
Sure, no problem.
cheers,
daniel
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-09-11 21:15 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120911210109.GZ7677-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Hi Tejun,
On 09/11/2012 11:01 PM, Tejun Heo wrote:
> Hello, Daniel.
>
> I generally like this but I still think it's too big a patch.
Yes, I agree it is a bit too big.
>> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
>> index c75e3f9..6bc460c 100644
>> --- a/net/core/netprio_cgroup.c
>> +++ b/net/core/netprio_cgroup.c
>> @@ -326,9 +326,7 @@ struct cgroup_subsys net_prio_subsys = {
>> .create = cgrp_create,
>> .destroy = cgrp_destroy,
>> .attach = net_prio_attach,
>> -#ifdef CONFIG_NETPRIO_CGROUP
>> .subsys_id = net_prio_subsys_id,
>> -#endif
>> .base_cftypes = ss_files,
>> .module = THIS_MODULE
>> };
>> @@ -366,10 +364,6 @@ static int __init init_cgroup_netprio(void)
>> ret = cgroup_load_subsys(&net_prio_subsys);
>> if (ret)
>> goto out;
>> -#ifndef CONFIG_NETPRIO_CGROUP
>> - smp_wmb();
>> - net_prio_subsys_id = net_prio_subsys.subsys_id;
>> -#endif
>>
>> register_netdevice_notifier(&netprio_device_notifier);
>>
>> @@ -386,11 +380,6 @@ static void __exit exit_cgroup_netprio(void)
>>
>> cgroup_unload_subsys(&net_prio_subsys);
>>
>> -#ifndef CONFIG_NETPRIO_CGROUP
>> - net_prio_subsys_id = -1;
>> - synchronize_rcu();
>
> For example, it's not evident the above is correct and it's burried
> with all other changes. Can't we just assign the fixed subsys ID to
> net_prio_subsys_id in this patch? This patch would be correct without
> any netprio changes, no?
If net_prio_subsys_id is changed to be an enum, then the compiler will
report an error:
error: lvalue required as left operand of assignment
that was the reason why I kept this change here. I think I just don't
get what you are trying to tell me.
> Please separate these changes and explain them.
I will do that as soon I figured out what you are telling me.
> BTW, people who use barriers of any kind without explicitly explaining
> what's going on need to be kicked hard in the ass. :(
I looked up the commit message when the synchronze_rcu() was added. It
was not really explaining it. I really spend a few hours starring at
this code.
thanks for the review,
daniel
^ permalink raw reply
* Re: [PATCH v3 0/8] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-09-11 21:11 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev, cgroups, Daniel Wagner, David S. Miller, Andrew Morton,
Eric Dumazet, Gao feng, Glauber Costa, Jamal Hadi Salim,
John Fastabend, Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <1347380774-9546-1-git-send-email-wagi@monom.org>
Hello,
On Tue, Sep 11, 2012 at 06:26:06PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> Hi,
>
> In this version I tried to concentrate on the main topic of this
> series, so I removed some of the things which were not really needed
> and I have to admit the result looks much better. So I hope that will
> simplify the review for you.
Other than the few nits I like the whole series and, given that all
the changes are entangled with cgroup core, I would like to route it
through cgroup/for-3.7 if nobody objects. Daniel, can you please
rebase on top of the following branch when sending out updated
version?
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.7
Li, can you please review these?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-09-11 21:08 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120911210109.GZ7677-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Tue, Sep 11, 2012 at 02:01:09PM -0700, Tejun Heo wrote:
> For example, it's not evident the above is correct and it's burried
> with all other changes. Can't we just assign the fixed subsys ID to
> net_prio_subsys_id in this patch? This patch would be correct without
> any netprio changes, no?
Oops, that was wrong. net_prio_subsys_id itself becomes constant.
Let's please better explain why the RCU trick removal is safe then.
Thanks.
--
tejun
^ permalink raw reply
* [PATCH] rtlwifi: Remove EXPERIMENTAL as pre-requisite for the drivers
From: Larry Finger @ 2012-09-11 21:04 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, Larry Finger, netdev
All of the rtlwifi-family of drivers have been in the kernel since 3.1
or earlier. The dependence on EXPERIMENTAL can be removed.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
John,
There is no rush on this patch. Inclusion in 3.7 will be fine.
Larry
---
drivers/net/wireless/rtlwifi/Kconfig | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/Kconfig b/drivers/net/wireless/rtlwifi/Kconfig
index cefac6a..6b28e92 100644
--- a/drivers/net/wireless/rtlwifi/Kconfig
+++ b/drivers/net/wireless/rtlwifi/Kconfig
@@ -1,6 +1,6 @@
config RTL8192CE
tristate "Realtek RTL8192CE/RTL8188CE Wireless Network Adapter"
- depends on MAC80211 && PCI && EXPERIMENTAL
+ depends on MAC80211 && PCI
select FW_LOADER
select RTLWIFI
select RTL8192C_COMMON
@@ -12,7 +12,7 @@ config RTL8192CE
config RTL8192SE
tristate "Realtek RTL8192SE/RTL8191SE PCIe Wireless Network Adapter"
- depends on MAC80211 && EXPERIMENTAL && PCI
+ depends on MAC80211 && PCI
select FW_LOADER
select RTLWIFI
---help---
@@ -23,7 +23,7 @@ config RTL8192SE
config RTL8192DE
tristate "Realtek RTL8192DE/RTL8188DE PCIe Wireless Network Adapter"
- depends on MAC80211 && EXPERIMENTAL && PCI
+ depends on MAC80211 && PCI
select FW_LOADER
select RTLWIFI
---help---
@@ -34,7 +34,7 @@ config RTL8192DE
config RTL8192CU
tristate "Realtek RTL8192CU/RTL8188CU USB Wireless Network Adapter"
- depends on MAC80211 && USB && EXPERIMENTAL
+ depends on MAC80211 && USB
select FW_LOADER
select RTLWIFI
select RTL8192C_COMMON
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-09-11 21:04 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <1347380774-9546-8-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
Hello, Daniel.
One more thing.
On Tue, Sep 11, 2012 at 06:26:13PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>
> WARNING: With this change it is not possible to load external built
> controllers anymore.
>
> In case where CONFIG_NETPRIO_CGROUP=m and CONFIG_NET_CLS_CGROUP=m is
> set, the type of the corresponding subsys_id should also be of type
> enum. Up to now, net_prio_subsys_id and net_cls_subsys_id would be an
> int in this configuration.
>
> With switching the macro definition IS_SUBSYS_ENABLED from IS_BUILTIN
> to IS_ENABLED, the subsys_id will always be enum for all
> subsystems. That means we need to remove all the code which assumes
> that net_prio_subsys_id and net_cls_subsys_id is of type int.
I don't think int or enum is the matter here. enum is an int. It's
whether the ID is allocated statically or dynamically. Can you please
update the description using those terms instead?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 2/2] iproute2: use libgenl in ipl2tp
From: Stephen Hemminger @ 2012-09-11 21:02 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev
In-Reply-To: <alpine.LFD.2.00.1209112304410.2023@ja.ssi.bg>
On Tue, 11 Sep 2012 23:22:41 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:
>
> Hello,
>
> On Tue, 11 Sep 2012, Stephen Hemminger wrote:
>
> > On Tue, 11 Sep 2012 12:04:34 +0300
> > Julian Anastasov <ja@ssi.bg> wrote:
> >
> > > Use the common code from libgenl.c to parse family.
> > >
> > > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> >
> > I applied these two but made some modifications:
> > 1. change to GENL_INITIALIZER
> > 2. use GENL_INITIALIZER
>
> I used 2 defines, so that in tcp_metrics.c I can
> use different initialization depending on the command.
> But as the structure is unnamed may be it is better to have
> single define (both defines merged as you proposed) and
> later just to update cmd and flags if needed.
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
Changes accepted any time.
^ permalink raw reply
* Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-09-11 21:01 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev, cgroups, Daniel Wagner, David S. Miller, Andrew Morton,
Eric Dumazet, Gao feng, Glauber Costa, Jamal Hadi Salim,
John Fastabend, Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <1347380774-9546-8-git-send-email-wagi@monom.org>
Hello, Daniel.
I generally like this but I still think it's too big a patch.
> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> index c75e3f9..6bc460c 100644
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -326,9 +326,7 @@ struct cgroup_subsys net_prio_subsys = {
> .create = cgrp_create,
> .destroy = cgrp_destroy,
> .attach = net_prio_attach,
> -#ifdef CONFIG_NETPRIO_CGROUP
> .subsys_id = net_prio_subsys_id,
> -#endif
> .base_cftypes = ss_files,
> .module = THIS_MODULE
> };
> @@ -366,10 +364,6 @@ static int __init init_cgroup_netprio(void)
> ret = cgroup_load_subsys(&net_prio_subsys);
> if (ret)
> goto out;
> -#ifndef CONFIG_NETPRIO_CGROUP
> - smp_wmb();
> - net_prio_subsys_id = net_prio_subsys.subsys_id;
> -#endif
>
> register_netdevice_notifier(&netprio_device_notifier);
>
> @@ -386,11 +380,6 @@ static void __exit exit_cgroup_netprio(void)
>
> cgroup_unload_subsys(&net_prio_subsys);
>
> -#ifndef CONFIG_NETPRIO_CGROUP
> - net_prio_subsys_id = -1;
> - synchronize_rcu();
For example, it's not evident the above is correct and it's burried
with all other changes. Can't we just assign the fixed subsys ID to
net_prio_subsys_id in this patch? This patch would be correct without
any netprio changes, no? Please separate these changes and explain
them.
BTW, people who use barriers of any kind without explicitly explaining
what's going on need to be kicked hard in the ass. :(
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 4/8] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT
From: Tejun Heo @ 2012-09-11 20:41 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, Gao feng, Jamal Hadi Salim, John Fastabend,
Li Zefan, Neil Horman
In-Reply-To: <1347380774-9546-5-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
Hello, Daniel.
Just one nit.
On Tue, Sep 11, 2012 at 06:26:10PM +0200, Daniel Wagner wrote:
> @@ -4502,10 +4507,13 @@ int __init cgroup_init_early(void)
> for (i = 0; i < CSS_SET_TABLE_SIZE; i++)
> INIT_HLIST_HEAD(&css_set_table[i]);
>
> - /* at bootup time, we don't worry about modular subsystems */
> - for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
> + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> struct cgroup_subsys *ss = subsys[i];
>
> + /* at bootup time, we don't worry about modular subsystems */
> + if (!ss || (ss && ss->module))
> + continue;
> +
The middle "ss" test is unnecessary. Control never gets there if
NULL.
> @@ -4538,9 +4546,12 @@ int __init cgroup_init(void)
> if (err)
> return err;
>
> - /* at bootup time, we don't worry about modular subsystems */
> - for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
> + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> struct cgroup_subsys *ss = subsys[i];
> +
> + /* at bootup time, we don't worry about modular subsystems */
> + if (!ss || (ss && ss->module))
> + continue;
Ditto.
> @@ -4735,13 +4746,16 @@ void cgroup_fork_callbacks(struct task_struct *child)
> {
> if (need_forkexit_callback) {
> int i;
> - /*
> - * forkexit callbacks are only supported for builtin
> - * subsystems, and the builtin section of the subsys array is
> - * immutable, so we don't need to lock the subsys array here.
> - */
> - for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
> + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> struct cgroup_subsys *ss = subsys[i];
> +
> + /*
> + * forkexit callbacks are only supported for
> + * builtin subsystems.
> + */
> + if (!ss || (ss && ss->module))
> + continue;
> +
Ditto.
> @@ -4846,12 +4860,13 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
> tsk->cgroups = &init_css_set;
>
> if (run_callbacks && need_forkexit_callback) {
> - /*
> - * modular subsystems can't use callbacks, so no need to lock
> - * the subsys array
> - */
> - for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
> + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> struct cgroup_subsys *ss = subsys[i];
> +
> + /* modular subsystems can't use callbacks */
> + if (!ss || (ss && ss->module))
> + continue;
> +
Ditto.
> @@ -5037,13 +5052,17 @@ static int __init cgroup_disable(char *str)
> while ((token = strsep(&str, ",")) != NULL) {
> if (!*token)
> continue;
> - /*
> - * cgroup_disable, being at boot time, can't know about module
> - * subsystems, so we don't worry about them.
> - */
> - for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
> + for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> struct cgroup_subsys *ss = subsys[i];
>
> + /*
> + * cgroup_disable, being at boot time, can't
> + * know about module subsystems, so we don't
> + * worry about them.
> + */
> + if (!ss || (ss && ss->module))
> + continue;
> +
Ditto.
Other than that,
Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 3/8] cgroup: net_prio: Do not define task_netpioidx() when not selected
From: Tejun Heo @ 2012-09-11 20:37 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, Gao feng, Jamal Hadi Salim, John Fastabend,
Li Zefan, Neil Horman
In-Reply-To: <1347380774-9546-4-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Tue, Sep 11, 2012 at 06:26:09PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>
> task_netprioidx() should not be defined in case the configuration is
> CONFIG_NETPRIO_CGROUP=n. The reason is that in a following patch the
> net_prio_subsys_id will only be defined if CONFIG_NETPRIO_CGROUP!=n.
> When net_prio is not built at all any callee should only get an empty
> task_netprioidx() without any references to net_prio_subsys_id.
>
> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> Cc: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
> Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
For 1-3.
Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 2/2] iproute2: use libgenl in ipl2tp
From: Julian Anastasov @ 2012-09-11 20:22 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20120911090908.605d1fbe@nehalam.linuxnetplumber.net>
Hello,
On Tue, 11 Sep 2012, Stephen Hemminger wrote:
> On Tue, 11 Sep 2012 12:04:34 +0300
> Julian Anastasov <ja@ssi.bg> wrote:
>
> > Use the common code from libgenl.c to parse family.
> >
> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
>
> I applied these two but made some modifications:
> 1. change to GENL_INITIALIZER
> 2. use GENL_INITIALIZER
I used 2 defines, so that in tcp_metrics.c I can
use different initialization depending on the command.
But as the structure is unnamed may be it is better to have
single define (both defines merged as you proposed) and
later just to update cmd and flags if needed.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox