* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Johannes Berg @ 2010-06-08 12:03 UTC (permalink / raw)
To: Kay Sievers; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1275998127.1899.38.camel@yio.site>
On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote:
> Ok, this needs testing.
>
> Please let me know, if that works for you so far.
Will do. Just a question: none of this seems to pin the module? So can I
be sure if I rmmod the module that the release function will still be
around etc.?
johannes
^ permalink raw reply
* Re: [PATCH nf-next-2.6] xt_rateest: Better struct xt_rateest layout
From: Patrick McHardy @ 2010-06-08 12:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Netfilter Development Mailinglist
In-Reply-To: <1275941837.2775.7.camel@edumazet-laptop>
On 07.06.2010 22:17, Eric Dumazet wrote:
> We currently dirty two cache lines in struct xt_rateest, this hurts SMP
> performance.
>
> This patch moves lock/bstats/rstats at beginning of structure so that
> they share a single cache line.
Applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Jarek Poplawski @ 2010-06-08 12:15 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <1275929761.2545.159.camel@edumazet-laptop>
Eric Dumazet wrote:
...
> [PATCH net-next-2.6] pkt_sched: gen_kill_estimator() rcu fixes
>
> gen_kill_estimator() API is fundamentaly wrong, since caller should make
> sure an RCU grace period is respected before freeing bstats or lock.
>
> This was partially addressed in commit 5d944c640b4 (gen_estimator:
> deadlock fix), but same problem exist for all gen_kill_estimator()
> users.
>
> Change its name to gen_kill_estimator_rcu() and change all callers to
> use RCU grace period before freeing bstats container.
>
> As a bonus, est_lock rwlock can be killed for good.
>
> Special thanks to Changli Gao for commenting on a previous patch, this
> made this bug clear to me.
>
Actually, I guess, Changli meant the bug introduced by your previous
patch by removing the est_lock. With this lock (and your commit 5d944)
bstats (and API) seem "fundamentaly" safe.
Jarek P.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Eric Dumazet @ 2010-06-08 12:27 UTC (permalink / raw)
To: Jarek Poplawski
Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <20100608121546.GA9392@ff.dom.local>
Le mardi 08 juin 2010 à 12:15 +0000, Jarek Poplawski a écrit :
>
> Actually, I guess, Changli meant the bug introduced by your previous
> patch by removing the est_lock. With this lock (and your commit 5d944)
> bstats (and API) seem "fundamentaly" safe.
>
Sorry, I have no idea of what you want to say, I cant find commit 5d944.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Jarek Poplawski @ 2010-06-08 12:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <1276000052.2475.307.camel@edumazet-laptop>
On Tue, Jun 08, 2010 at 02:27:32PM +0200, Eric Dumazet wrote:
> Le mardi 08 juin 2010 ?? 12:15 +0000, Jarek Poplawski a écrit :
>
> >
> > Actually, I guess, Changli meant the bug introduced by your previous
> > patch by removing the est_lock. With this lock (and your commit 5d944)
> > bstats (and API) seem "fundamentaly" safe.
> >
>
> Sorry, I have no idea of what you want to say, I cant find commit 5d944.
>
Sorry, I meant the commit mentioned in your changelog which was quoted.
> This was partially addressed in commit 5d944c640b4 (gen_estimator:
> deadlock fix), but same problem exist for all gen_kill_estimator()
> users.
Jarek P.
^ permalink raw reply
* AF_PACKET meets AF_UNIX
From: Sebastian Krahmer @ 2010-06-08 12:43 UTC (permalink / raw)
To: netdev
Hi
I made a small fix for 2.6.34 to have something like
"tcpdump -i unix". For debugging purposes (DBUS etc)
it would be really helpfull. The sockaddr_ll struct
contains the socket's ino and the senders PID in
ifindex, so all necessary infos can be found
out via /proc/net/unix in userland.
The patch goes here: http://www.suse.de/~krahmer/unix-monitor-2.6.34.tgz
If you dont like any constructions in it please let me know,
so I can fix it and it can be accepted upstream :)
cya,
Sebastian
--
~
~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer@suse.de - SuSE Security Team
~ SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg)
^ permalink raw reply
* Re: 2.6.35-rc2-git1 - include/linux/cgroup.h:534 invoked rcu_dereference_check() without protection!
From: Miles Lane @ 2010-06-08 13:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: paulmck, Vivek Goyal, Eric Paris, Lai Jiangshan, Ingo Molnar,
LKML, nauman, eric.dumazet, netdev, Jens Axboe, Gui Jianfeng,
Li Zefan, Johannes Berg
In-Reply-To: <1275986441.5408.111.camel@twins>
On Tue, Jun 8, 2010 at 4:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-06-08 at 00:16 -0400, Miles Lane wrote:
>> On Mon, Jun 7, 2010 at 8:19 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Mon, Jun 07, 2010 at 02:14:25PM -0400, Miles Lane wrote:
>> >> Hi All,
>> >>
>> >> I just reproduced a warning I reported quite a while ago. Is a patch
>> >> for this in the pipeline?
>> >
>> > I proposed a patch, thinking that it was a false positive. Peter Zijlstra
>> > pointed out that there was a real race, and proposed an alternative patch,
>> > which may be found at http://lkml.org/lkml/2010/4/22/603.
>> >
>> > Could you please test Peter's patch and let us know if it cures the problem?
>> >
>
> Gah, this task_group() stuff is annoying, how about something like the
> below which teaches task_group() about the task_rq()->lock rule?
>
> ---
> include/linux/cgroup.h | 20 +++++++++++----
> kernel/sched.c | 61 +++++++++++++++++++++++++----------------------
> 2 files changed, 46 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 0c62160..1efd212 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -525,13 +525,21 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
> return cgrp->subsys[subsys_id];
> }
>
> -static inline struct cgroup_subsys_state *task_subsys_state(
> - struct task_struct *task, int subsys_id)
> +/*
> + * function to get the cgroup_subsys_state which allows for extra
> + * rcu_dereference_check() conditions, such as locks used during the
> + * cgroup_subsys::attach() methods.
> + */
> +#define task_subsys_state_check(task, subsys_id, __c) \
> + rcu_dereference_check(task->cgroups->subsys[subsys_id], \
> + rcu_read_lock_held() || \
> + lockdep_is_held(&task->alloc_lock) || \
> + cgroup_lock_is_held() || (__c))
> +
> +static inline struct cgroup_subsys_state *
> +task_subsys_state(struct task_struct *task, int subsys_id)
> {
> - return rcu_dereference_check(task->cgroups->subsys[subsys_id],
> - rcu_read_lock_held() ||
> - lockdep_is_held(&task->alloc_lock) ||
> - cgroup_lock_is_held());
> + return task_subsys_state_check(task, subsys_id, false);
> }
>
> static inline struct cgroup* task_cgroup(struct task_struct *task,
> diff --git a/kernel/sched.c b/kernel/sched.c
> index f8b8996..e01bb45 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -306,32 +306,26 @@ static int init_task_group_load = INIT_TASK_GROUP_LOAD;
> */
> struct task_group init_task_group;
>
> -/* return group to which a task belongs */
> +/*
> + * Return the group to which this tasks belongs.
> + *
> + * We use task_subsys_state_check() and extend the RCU verification
> + * with lockdep_is_held(&task_rq(p)->lock) because cpu_cgroup_attach()
> + * holds that lock for each task it moves into the cgroup. Therefore
> + * by holding that lock, we pin the task to the current cgroup.
> + */
> static inline struct task_group *task_group(struct task_struct *p)
> {
> - struct task_group *tg;
> + struct cgroup_subsys_state *css;
>
> -#ifdef CONFIG_CGROUP_SCHED
> - tg = container_of(task_subsys_state(p, cpu_cgroup_subsys_id),
> - struct task_group, css);
> -#else
> - tg = &init_task_group;
> -#endif
> - return tg;
> + css = task_subsys_state_check(p, cpu_cgroup_subsys_id,
> + lockdep_is_held(&task_rq(p)->lock));
> + return container_of(css, struct task_group, css);
> }
>
> /* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
> static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> {
> - /*
> - * Strictly speaking this rcu_read_lock() is not needed since the
> - * task_group is tied to the cgroup, which in turn can never go away
> - * as long as there are tasks attached to it.
> - *
> - * However since task_group() uses task_subsys_state() which is an
> - * rcu_dereference() user, this quiets CONFIG_PROVE_RCU.
> - */
> - rcu_read_lock();
> #ifdef CONFIG_FAIR_GROUP_SCHED
> p->se.cfs_rq = task_group(p)->cfs_rq[cpu];
> p->se.parent = task_group(p)->se[cpu];
> @@ -341,7 +335,6 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> p->rt.rt_rq = task_group(p)->rt_rq[cpu];
> p->rt.parent = task_group(p)->rt_se[cpu];
> #endif
> - rcu_read_unlock();
> }
>
> #else
> @@ -4465,16 +4458,6 @@ recheck:
> }
>
> if (user) {
> -#ifdef CONFIG_RT_GROUP_SCHED
> - /*
> - * Do not allow realtime tasks into groups that have no runtime
> - * assigned.
> - */
> - if (rt_bandwidth_enabled() && rt_policy(policy) &&
> - task_group(p)->rt_bandwidth.rt_runtime == 0)
> - return -EPERM;
> -#endif
> -
> retval = security_task_setscheduler(p, policy, param);
> if (retval)
> return retval;
> @@ -4490,6 +4473,26 @@ recheck:
> * runqueue lock must be held.
> */
> rq = __task_rq_lock(p);
> +
> + retval = 0;
> +#ifdef CONFIG_RT_GROUP_SCHED
> + if (user) {
> + /*
> + * Do not allow realtime tasks into groups that have no runtime
> + * assigned.
> + */
> + if (rt_bandwidth_enabled() && rt_policy(policy) &&
> + task_group(p)->rt_bandwidth.rt_runtime == 0)
> + retval = -EPERM;
> +
> + if (retval) {
> + __task_rq_unlock(rq);
> + raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> + return retval;
> + }
> + }
> +#endif
> +
> /* recheck policy now with rq lock held */
> if (unlikely(oldpolicy != -1 && oldpolicy != p->policy)) {
> policy = oldpolicy = -1;
>
>
CC kernel/sched.o
kernel/sched.c: In function ‘task_group’:
kernel/sched.c:321: error: implicit declaration of function ‘task_rq’
kernel/sched.c:321: error: invalid type argument of ‘->’ (have ‘int’)
make[1]: *** [kernel/sched.o] Error 1
I had to apply with fuzz. Did it mess up?
static inline struct task_group *task_group(struct task_struct *p)
{
struct cgroup_subsys_state *css;
css = task_subsys_state_check(p, cpu_cgroup_subsys_id,
lockdep_is_held(&task_rq(p)->lock));
return container_of(css, struct task_group, css);
}
^ permalink raw reply
* Re: BUG: double spinlock in "drivers/net/3c505.c"
From: Christoph Fritz @ 2010-06-08 13:26 UTC (permalink / raw)
To: Alexander Strakh
Cc: Philip Blundell, Craig Southeren, Andrew Tridgell, Alan Cox,
netdev, linux-kernel
In-Reply-To: <201006071517.28744.strakh@ispras.ru>
On Mon, 2010-06-07 at 15:17 +0400, Alexander Strakh wrote:
> KERNEL_VERSION: 2.6.35-rc1
> SUBJECT: duble spinlock in function elp_start_xmit
Not only in elp_start_xmit. This driver is for a pretty old and slow isa
ethernet card and I think nobody cares. To quote a comment from the
source: "[...] the concurrency protection is particularly awful".
Thanks,
Christoph
^ permalink raw reply
* Re: 2.6.35-rc2-git1 - include/linux/cgroup.h:534 invoked rcu_dereference_check() without protection!
From: Peter Zijlstra @ 2010-06-08 13:34 UTC (permalink / raw)
To: Miles Lane
Cc: paulmck, Vivek Goyal, Eric Paris, Lai Jiangshan, Ingo Molnar,
LKML, nauman, eric.dumazet, netdev, Jens Axboe, Gui Jianfeng,
Li Zefan, Johannes Berg
In-Reply-To: <AANLkTimXVlgzHivCQberLdrFP8BH_nJzt8_ozUNHG_aI@mail.gmail.com>
On Tue, 2010-06-08 at 09:14 -0400, Miles Lane wrote:
> CC kernel/sched.o
> kernel/sched.c: In function ‘task_group’:
> kernel/sched.c:321: error: implicit declaration of function ‘task_rq’
> kernel/sched.c:321: error: invalid type argument of ‘->’ (have ‘int’)
> make[1]: *** [kernel/sched.o] Error 1
>
> I had to apply with fuzz. Did it mess up?
No, I probably did.. task_rq() is defined on line 636 or thereabouts,
and this function landed around line 320.
Ahh, and it compiled here because I have CGROUP_SCHED=y, but
PROVE_RCU=n, so that whole check expression disappears and is never
evaluated...
/me fixes
---
Subject: sched: PROVE_RCU vs cpu_cgroup
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue Jun 08 11:40:42 CEST 2010
PROVE_RCU has a few issues with the cpu_cgroup because the scheduler
typically holds rq->lock around the css rcu derefs but the generic
cgroup code doesn't (and can't) know about that lock.
Provide means to add extra checks to the css dereference and use that
in the scheduler to annotate its users.
The addition of rq->lock to these checks is correct because the
cgroup_subsys::attach() method takes the rq->lock for each task it
moves, therefore by holding that lock, we ensure the task is pinned to
the current cgroup and the RCU dereference is valid.
That leaves one genuine race in __sched_setscheduler() where we used
task_group() without holding any of the required locks and thus raced
with the cgroup code. Solve this by moving the check under the rq->lock.
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
include/linux/cgroup.h | 20 +++++---
kernel/sched.c | 115 +++++++++++++++++++++++++------------------------
2 files changed, 73 insertions(+), 62 deletions(-)
Index: linux-2.6/include/linux/cgroup.h
===================================================================
--- linux-2.6.orig/include/linux/cgroup.h
+++ linux-2.6/include/linux/cgroup.h
@@ -525,13 +525,21 @@ static inline struct cgroup_subsys_state
return cgrp->subsys[subsys_id];
}
-static inline struct cgroup_subsys_state *task_subsys_state(
- struct task_struct *task, int subsys_id)
+/*
+ * function to get the cgroup_subsys_state which allows for extra
+ * rcu_dereference_check() conditions, such as locks used during the
+ * cgroup_subsys::attach() methods.
+ */
+#define task_subsys_state_check(task, subsys_id, __c) \
+ rcu_dereference_check(task->cgroups->subsys[subsys_id], \
+ rcu_read_lock_held() || \
+ lockdep_is_held(&task->alloc_lock) || \
+ cgroup_lock_is_held() || (__c))
+
+static inline struct cgroup_subsys_state *
+task_subsys_state(struct task_struct *task, int subsys_id)
{
- return rcu_dereference_check(task->cgroups->subsys[subsys_id],
- rcu_read_lock_held() ||
- lockdep_is_held(&task->alloc_lock) ||
- cgroup_lock_is_held());
+ return task_subsys_state_check(task, subsys_id, false);
}
static inline struct cgroup* task_cgroup(struct task_struct *task,
Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -306,52 +306,6 @@ static int init_task_group_load = INIT_T
*/
struct task_group init_task_group;
-/* return group to which a task belongs */
-static inline struct task_group *task_group(struct task_struct *p)
-{
- struct task_group *tg;
-
-#ifdef CONFIG_CGROUP_SCHED
- tg = container_of(task_subsys_state(p, cpu_cgroup_subsys_id),
- struct task_group, css);
-#else
- tg = &init_task_group;
-#endif
- return tg;
-}
-
-/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
-static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
-{
- /*
- * Strictly speaking this rcu_read_lock() is not needed since the
- * task_group is tied to the cgroup, which in turn can never go away
- * as long as there are tasks attached to it.
- *
- * However since task_group() uses task_subsys_state() which is an
- * rcu_dereference() user, this quiets CONFIG_PROVE_RCU.
- */
- rcu_read_lock();
-#ifdef CONFIG_FAIR_GROUP_SCHED
- p->se.cfs_rq = task_group(p)->cfs_rq[cpu];
- p->se.parent = task_group(p)->se[cpu];
-#endif
-
-#ifdef CONFIG_RT_GROUP_SCHED
- p->rt.rt_rq = task_group(p)->rt_rq[cpu];
- p->rt.parent = task_group(p)->rt_se[cpu];
-#endif
- rcu_read_unlock();
-}
-
-#else
-
-static inline void set_task_rq(struct task_struct *p, unsigned int cpu) { }
-static inline struct task_group *task_group(struct task_struct *p)
-{
- return NULL;
-}
-
#endif /* CONFIG_CGROUP_SCHED */
/* CFS-related fields in a runqueue */
@@ -644,6 +598,49 @@ static inline int cpu_of(struct rq *rq)
#define cpu_curr(cpu) (cpu_rq(cpu)->curr)
#define raw_rq() (&__raw_get_cpu_var(runqueues))
+#ifdef CONFIG_CGROUP_SCHED
+
+/*
+ * Return the group to which this tasks belongs.
+ *
+ * We use task_subsys_state_check() and extend the RCU verification
+ * with lockdep_is_held(&task_rq(p)->lock) because cpu_cgroup_attach()
+ * holds that lock for each task it moves into the cgroup. Therefore
+ * by holding that lock, we pin the task to the current cgroup.
+ */
+static inline struct task_group *task_group(struct task_struct *p)
+{
+ struct cgroup_subsys_state *css;
+
+ css = task_subsys_state_check(p, cpu_cgroup_subsys_id,
+ lockdep_is_held(&task_rq(p)->lock));
+ return container_of(css, struct task_group, css);
+}
+
+/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
+static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
+{
+#ifdef CONFIG_FAIR_GROUP_SCHED
+ p->se.cfs_rq = task_group(p)->cfs_rq[cpu];
+ p->se.parent = task_group(p)->se[cpu];
+#endif
+
+#ifdef CONFIG_RT_GROUP_SCHED
+ p->rt.rt_rq = task_group(p)->rt_rq[cpu];
+ p->rt.parent = task_group(p)->rt_se[cpu];
+#endif
+}
+
+#else /* CONFIG_CGROUP_SCHED */
+
+static inline void set_task_rq(struct task_struct *p, unsigned int cpu) { }
+static inline struct task_group *task_group(struct task_struct *p)
+{
+ return NULL;
+}
+
+#endif /* CONFIG_CGROUP_SCHED */
+
inline void update_rq_clock(struct rq *rq)
{
if (!rq->skip_clock_update)
@@ -4465,16 +4462,6 @@ recheck:
}
if (user) {
-#ifdef CONFIG_RT_GROUP_SCHED
- /*
- * Do not allow realtime tasks into groups that have no runtime
- * assigned.
- */
- if (rt_bandwidth_enabled() && rt_policy(policy) &&
- task_group(p)->rt_bandwidth.rt_runtime == 0)
- return -EPERM;
-#endif
-
retval = security_task_setscheduler(p, policy, param);
if (retval)
return retval;
@@ -4490,6 +4477,22 @@ recheck:
* runqueue lock must be held.
*/
rq = __task_rq_lock(p);
+
+#ifdef CONFIG_RT_GROUP_SCHED
+ if (user) {
+ /*
+ * Do not allow realtime tasks into groups that have no runtime
+ * assigned.
+ */
+ if (rt_bandwidth_enabled() && rt_policy(policy) &&
+ task_group(p)->rt_bandwidth.rt_runtime == 0) {
+ __task_rq_unlock(rq);
+ raw_spin_unlock_irqrestore(&p->pi_lock, flags);
+ return -EPERM;
+ }
+ }
+#endif
+
/* recheck policy now with rq lock held */
if (unlikely(oldpolicy != -1 && oldpolicy != p->policy)) {
policy = oldpolicy = -1;
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Kay Sievers @ 2010-06-08 13:54 UTC (permalink / raw)
To: Johannes Berg; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1275998603.3706.118.camel@jlt3.sipsolutions.net>
On Tue, Jun 8, 2010 at 14:03, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote:
>
>> Ok, this needs testing.
>>
>> Please let me know, if that works for you so far.
>
> Will do. Just a question: none of this seems to pin the module? So can I
> be sure if I rmmod the module that the release function will still be
> around etc.?
Oh, sorry, that's something very specific to network devices, that the
module can be unloaded while the devices it has created are still in
use. I am not sure right now what's needed to make this work in a
single module.
Kay
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Johannes Berg @ 2010-06-08 14:06 UTC (permalink / raw)
To: Kay Sievers; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <AANLkTinev96yxFCFSdDdT89jjgYR4ChUCMU2-aSj8kkh@mail.gmail.com>
On Tue, 2010-06-08 at 15:54 +0200, Kay Sievers wrote:
> On Tue, Jun 8, 2010 at 14:03, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote:
> >
> >> Ok, this needs testing.
> >>
> >> Please let me know, if that works for you so far.
> >
> > Will do. Just a question: none of this seems to pin the module? So can I
> > be sure if I rmmod the module that the release function will still be
> > around etc.?
>
> Oh, sorry, that's something very specific to network devices, that the
> module can be unloaded while the devices it has created are still in
> use. I am not sure right now what's needed to make this work in a
> single module.
Well they will be unregistered and everything, but once all the netdevs
are gone etc. the devices you create in the patch might stick around
because somebody has them open in sysfs, and I see nothing that would
pin the module in that case?
johannes
^ permalink raw reply
* Re: [PATCH nf-next-2.6] conntrack: IPS_UNTRACKED bit
From: Patrick McHardy @ 2010-06-08 14:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Changli Gao, Netfilter Developers, netdev
In-Reply-To: <1275668732.2482.201.camel@edumazet-laptop>
On 04.06.2010 18:25, Eric Dumazet wrote:
> [PATCH nf-next-2.6] conntrack: IPS_UNTRACKED bit
>
> NOTRACK makes all cpus share a cache line on nf_conntrack_untracked
> twice per packet. This is bad for performance.
> __read_mostly annotation is also a bad choice.
>
> This patch introduces IPS_UNTRACKED bit so that we can use later a
> per_cpu untrack structure more easily.
>
> A new helper, nf_ct_untracked_get() returns a pointer to
> nf_conntrack_untracked.
>
> Another one, nf_ct_untracked_status_or() is used by nf_nat_init() to add
> IPS_NAT_DONE_MASK bits to untracked status.
>
> nf_ct_is_untracked() prototype is changed to work on a nf_conn pointer.
Applied, thanks Eric.
^ permalink raw reply
* Re: RFS seems to have incompatiblities with bridged vlans
From: Eric Dumazet @ 2010-06-08 14:18 UTC (permalink / raw)
To: John Fastabend
Cc: Stephen Hemminger, Peter Lieven, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <4C0D7312.1020300@intel.com>
Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
> There is always a possibility that the underlying device sets the
> queue_mapping to be greater then num_cpus. Also I suspect the same
> issue exists with bonding devices. Maybe something like the following
> is worth while? compile tested only,
>
> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>
> recv path reassigns skb->dev without sanity checking the
> queue_mapping field. This can result in the queue_mapping
> field being set incorrectly if the new dev supports less
> queues then the underlying device.
>
> This patch just resets the queue_mapping to 0 which should
> resolve this issue? Any thoughts?
>
> The same issue could happen on bonding devices as well.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>
> net/8021q/vlan_core.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index bd537fc..ad309f8 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
> vlan_group *grp,
> if (!skb->dev)
> goto drop;
>
> + if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
> + skb_set_queue_mapping(skb, 0);
> +
> return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>
> drop:
> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
> vlan_group *grp,
> if (!skb->dev)
> goto drop;
>
> + if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
> + skb_set_queue_mapping(skb, 0);
> +
> for (p = napi->gro_list; p; p = p->next) {
> NAPI_GRO_CB(p)->same_flow =
> p->dev == skb->dev && !compare_ether_header(
> --
Only a workaround, added in hot path in a otherwise 'good' driver
(multiqueue enabled and ready)
eth0 -------> bond / bridge ---------> vlan.id
(nbtxq=8) (ntxbq=1) (nbtxq=X)
X is capped to 1 because of bond/bridge, while bond has no "queue"
(LLTX driver)
Solutions :
1) queue_mapping could be silently tested in get_rps_cpu()...
diff --git a/net/core/dev.c b/net/core/dev.c
index 6f330ce..3a3f7f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
if (skb_rx_queue_recorded(skb)) {
u16 index = skb_get_rx_queue(skb);
- if (unlikely(index >= dev->num_rx_queues)) {
- if (net_ratelimit()) {
- pr_warning("%s received packet on queue "
- "%u, but number of RX queues is %u\n",
- dev->name, index, dev->num_rx_queues);
- }
- goto done;
- }
+ if (WARN_ONCE(index >= dev->num_rx_queues,
+ KERN_WARNING "%s received packet on queue %u, "
+ "but number of RX queues is %u\n",
+ dev->name, index, dev->num_rx_queues))
+ index %= dev->num_rx_queues;
rxqueue = dev->_rx + index;
} else
rxqueue = dev->_rx;
2) bond/bridge should setup more queues, just in case.
We probably need to be able to make things more dynamic,
(propagate nbtxq between layers) but not for 2.6.35
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5e12462..ce813dd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
rtnl_lock();
- bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
- bond_setup);
+ bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
+ bond_setup, max(64, nr_cpu_ids));
if (!bond_dev) {
pr_err("%s: eek! can't alloc netdev!\n", name);
rtnl_unlock();
^ permalink raw reply related
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Kay Sievers @ 2010-06-08 14:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1276005970.3706.132.camel@jlt3.sipsolutions.net>
On Tue, Jun 8, 2010 at 16:06, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-06-08 at 15:54 +0200, Kay Sievers wrote:
>> On Tue, Jun 8, 2010 at 14:03, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote:
>> >
>> >> Ok, this needs testing.
>> >>
>> >> Please let me know, if that works for you so far.
>> >
>> > Will do. Just a question: none of this seems to pin the module? So can I
>> > be sure if I rmmod the module that the release function will still be
>> > around etc.?
>>
>> Oh, sorry, that's something very specific to network devices, that the
>> module can be unloaded while the devices it has created are still in
>> use. I am not sure right now what's needed to make this work in a
>> single module.
>
> Well they will be unregistered and everything, but once all the netdevs
> are gone etc. the devices you create in the patch might stick around
> because somebody has them open in sysfs, and I see nothing that would
> pin the module in that case?
That's what I mean, I have no idea how to solve that with network
devices. I don't think any other subsytem allows to unload the module,
when devices, the module has created, are in use.
With this patch, it is likely to fail, even without sysfs, just when
the netif is still in up, and the module is unloaded.
The current code uses the in-core class_create() logic, which was only
meant for devices with a device node, and which is cleaned up by the
core itself. That's why this issue never appeared before.
But as said, I have no idea how to solve that with a single module. It
might not work at all without moving stuff into the core. That people
use device_create() with no major/minor might indicate that something
else is needed here. :)
Kay
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Johannes Berg @ 2010-06-08 14:26 UTC (permalink / raw)
To: Kay Sievers; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <AANLkTikB0Amzi61JLxdY5HL-2s_nbYo3JH8vXooXzHdQ@mail.gmail.com>
On Tue, 2010-06-08 at 16:21 +0200, Kay Sievers wrote:
> > Well they will be unregistered and everything, but once all the netdevs
> > are gone etc. the devices you create in the patch might stick around
> > because somebody has them open in sysfs, and I see nothing that would
> > pin the module in that case?
>
> That's what I mean, I have no idea how to solve that with network
> devices. I don't think any other subsytem allows to unload the module,
> when devices, the module has created, are in use.
You're right ... the would only be unregistered from your release, which
would happen after the module is long gone ...
> The current code uses the in-core class_create() logic, which was only
> meant for devices with a device node, and which is cleaned up by the
> core itself. That's why this issue never appeared before.
>
> But as said, I have no idea how to solve that with a single module. It
> might not work at all without moving stuff into the core. That people
> use device_create() with no major/minor might indicate that something
> else is needed here. :)
So ... can we apply Eric's patch for now then?
johannes
^ permalink raw reply
* Re: [PATCH nf-next-2.6 2/2] conntrack: per_cpu untracking
From: Patrick McHardy @ 2010-06-08 14:29 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Changli Gao, Netfilter Developers, netdev
In-Reply-To: <1275682522.2490.6.camel@edumazet-laptop>
On 04.06.2010 22:15, Eric Dumazet wrote:
> NOTRACK makes all cpus share a cache line on nf_conntrack_untracked
> twice per packet, slowing down performance.
>
> This patch converts it to a per_cpu variable.
>
> We assume same cpu is used for a given packet, entering and exiting the
> NOTRACK state.
That doesn't seem to be a valid assumption, the conntrack entry is
attached to the skb and processing in the output path might get
preempted and rescheduled to a different CPU.
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Kay Sievers @ 2010-06-08 14:47 UTC (permalink / raw)
To: Johannes Berg; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <1276007174.3706.133.camel@jlt3.sipsolutions.net>
On Tue, Jun 8, 2010 at 16:26, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-06-08 at 16:21 +0200, Kay Sievers wrote:
>
>> > Well they will be unregistered and everything, but once all the netdevs
>> > are gone etc. the devices you create in the patch might stick around
>> > because somebody has them open in sysfs, and I see nothing that would
>> > pin the module in that case?
>>
>> That's what I mean, I have no idea how to solve that with network
>> devices. I don't think any other subsytem allows to unload the module,
>> when devices, the module has created, are in use.
>
> You're right ... the would only be unregistered from your release, which
> would happen after the module is long gone ...
>
>> The current code uses the in-core class_create() logic, which was only
>> meant for devices with a device node, and which is cleaned up by the
>> core itself. That's why this issue never appeared before.
>>
>> But as said, I have no idea how to solve that with a single module. It
>> might not work at all without moving stuff into the core. That people
>> use device_create() with no major/minor might indicate that something
>> else is needed here. :)
>
> So ... can we apply Eric's patch for now then?
It might break other stuff we don't know about yet. Just like we did
not know about what things hwsim is doing here. :)
Eric's patch change the sysfs layout and insert directories into the
device path, and stuff which hardcodes things, or accesses devices
with ../<name> will break.
I don't mind trying it, but I wouldn't be surprised if that needs to
be reverted when issues caused by this appear.
The hwsim issues are caused by the current hwsim code, by doing things
it should not do. Class devices of different classes must never be
stacked (the core should not allow that in the first place). Class
devices must never have a driver assigned behind its back. Also
device_create() should not be used for devices without a major/minor
(but that seems to be done in several other places too).
To fix the hwsim driver core interaction, core changes will probably
be needed to allow network modules to be removed while their devices
are active. That's something which seems not to work for bus devices
currently.
Kay
^ permalink raw reply
* Re: [PATCH nf-next-2.6 2/2] conntrack: per_cpu untracking
From: Eric Dumazet @ 2010-06-08 14:52 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Changli Gao, Netfilter Developers, netdev
In-Reply-To: <4C0E53C4.7090308@trash.net>
Le mardi 08 juin 2010 à 16:29 +0200, Patrick McHardy a écrit :
> On 04.06.2010 22:15, Eric Dumazet wrote:
> > NOTRACK makes all cpus share a cache line on nf_conntrack_untracked
> > twice per packet, slowing down performance.
> >
> > This patch converts it to a per_cpu variable.
> >
> > We assume same cpu is used for a given packet, entering and exiting the
> > NOTRACK state.
>
> That doesn't seem to be a valid assumption, the conntrack entry is
> attached to the skb and processing in the output path might get
> preempted and rescheduled to a different CPU.
Thats unfortunate.
Ok, only choice then is to not change refcount on the untracked ct, and
keep a shared (read only after setup time) untrack structure.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC][PATCH] Fix another namespace issue with devices assigned to classes
From: Johannes Berg @ 2010-06-08 15:06 UTC (permalink / raw)
To: Kay Sievers; +Cc: Eric W. Biederman, Greg KH, netdev
In-Reply-To: <AANLkTikNEzJHPrOUK6DKMoWvcblTBInaIhVbk96OtZxe@mail.gmail.com>
On Tue, 2010-06-08 at 16:47 +0200, Kay Sievers wrote:
> > So ... can we apply Eric's patch for now then?
>
> It might break other stuff we don't know about yet. Just like we did
> not know about what things hwsim is doing here. :)
True.
> The hwsim issues are caused by the current hwsim code, by doing things
> it should not do. Class devices of different classes must never be
> stacked (the core should not allow that in the first place). Class
> devices must never have a driver assigned behind its back. Also
> device_create() should not be used for devices without a major/minor
> (but that seems to be done in several other places too).
Back when hwsim was written that would have been useful feedback to
whoever did ... now, not so much.
> To fix the hwsim driver core interaction, core changes will probably
> be needed to allow network modules to be removed while their devices
> are active. That's something which seems not to work for bus devices
> currently.
Well it just needs to pin the module refcount from the bus and/or device
struct. That seems not too hard?
johannes
^ permalink raw reply
* Re: [Linux-ATM-General] RX/close vcc race with solos/atmtcp/usbatm/he
From: Chas Williams (CONTRACTOR) @ 2010-06-08 15:05 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-atm-general, netdev
In-Reply-To: <1275943792.17903.5119.camel@macbook.infradead.org>
In message <1275943792.17903.5119.camel@macbook.infradead.org>,David Woodhouse
writes:
>On Mon, 2010-06-07 at 12:37 -0400, Chas Williams (CONTRACTOR) wrote:
>> i dont understand. if you do a sock_hold() in find_vcc(), and then call
>> vcc->push() you should be able to call vcc->push() and then sock_put().
>
>Holding the reference doesn't stop the problem. The problem is
>
> vcc_release()
> --> vcc_destroy_socket()
> --> br2684_push(vcc, NULL)
> sets vcc->user_back = NULL
> (which it what causes the oops when try try to feed it any
> subsequent packets).
>
> Only _later_ does vcc_release() call sock_put().
hmm... perhaps this routine needs to take the vcc_sklist_lock because
it is going to modify the vcc. or we need to use locking on the vcc
itself. it seems like the lock in vcc_remove_socket() just needs move
up one routine to encompass this whole 'closing' operation. the vcc
is going away. we dont want people to be able to find it and use it.
it is not enough to just flip the flags on the vcc obviously.
you took a reference to an object inside a hashed list and didnt do
anything to prevent the object from leaving the hashed list. that is
stil not correct IMHO.
^ permalink raw reply
* Re: [PATCH nf-next-2.6 2/2] conntrack: per_cpu untracking
From: Eric Dumazet @ 2010-06-08 15:12 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Changli Gao, Netfilter Developers, netdev
In-Reply-To: <1276008733.2486.177.camel@edumazet-laptop>
Le mardi 08 juin 2010 à 16:52 +0200, Eric Dumazet a écrit :
> Le mardi 08 juin 2010 à 16:29 +0200, Patrick McHardy a écrit :
> > On 04.06.2010 22:15, Eric Dumazet wrote:
> > > NOTRACK makes all cpus share a cache line on nf_conntrack_untracked
> > > twice per packet, slowing down performance.
> > >
> > > This patch converts it to a per_cpu variable.
> > >
> > > We assume same cpu is used for a given packet, entering and exiting the
> > > NOTRACK state.
> >
> > That doesn't seem to be a valid assumption, the conntrack entry is
> > attached to the skb and processing in the output path might get
> > preempted and rescheduled to a different CPU.
>
> Thats unfortunate.
>
> Ok, only choice then is to not change refcount on the untracked ct, and
> keep a shared (read only after setup time) untrack structure.
>
>
Oh well, re-reading my patch, I dont see why I said this in Changelog :)
We lazily select the untrack structure in one cpu, then keep the pointer
to this untrack structure, attached to ct.
The (still atomic) increment / decrement of refcount is done on the
saved pointer, not on actual per_cpu structure.
So if a packet is rescheduled on a different CPU, second cpu will "only"
dirty cache line of other cpu, it probably almost never happens...
Thanks
[PATCH nf-next-2.6 2/2] conntrack: per_cpu untracking
NOTRACK makes all cpus share a cache line on nf_conntrack_untracked
twice per packet, slowing down performance.
This patch converts it to a per_cpu variable.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/netfilter/nf_conntrack.h | 5 +--
net/netfilter/nf_conntrack_core.c | 36 ++++++++++++++++++-------
2 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 3bc38c7..84a4b6f 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -261,11 +261,10 @@ extern s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
u32 seq);
/* Fake conntrack entry for untracked connections */
+DECLARE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
static inline struct nf_conn *nf_ct_untracked_get(void)
{
- extern struct nf_conn nf_conntrack_untracked;
-
- return &nf_conntrack_untracked;
+ return &__raw_get_cpu_var(nf_conntrack_untracked);
}
extern void nf_ct_untracked_status_or(unsigned long bits);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 6c1da21..9c66141 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -62,8 +62,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_htable_size);
unsigned int nf_conntrack_max __read_mostly;
EXPORT_SYMBOL_GPL(nf_conntrack_max);
-struct nf_conn nf_conntrack_untracked;
-EXPORT_SYMBOL_GPL(nf_conntrack_untracked);
+DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
+EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
static int nf_conntrack_hash_rnd_initted;
static unsigned int nf_conntrack_hash_rnd;
@@ -1183,10 +1183,21 @@ static void nf_ct_release_dying_list(struct net *net)
spin_unlock_bh(&nf_conntrack_lock);
}
+static int untrack_refs(void)
+{
+ int cnt = 0, cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct nf_conn *ct = &per_cpu(nf_conntrack_untracked, cpu);
+
+ cnt += atomic_read(&ct->ct_general.use) - 1;
+ }
+ return cnt;
+}
+
static void nf_conntrack_cleanup_init_net(void)
{
- /* wait until all references to nf_conntrack_untracked are dropped */
- while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
+ while (untrack_refs() > 0)
schedule();
nf_conntrack_helper_fini();
@@ -1323,14 +1334,17 @@ module_param_call(hashsize, nf_conntrack_set_hashsize, param_get_uint,
void nf_ct_untracked_status_or(unsigned long bits)
{
- nf_conntrack_untracked.status |= bits;
+ int cpu;
+
+ for_each_possible_cpu(cpu)
+ per_cpu(nf_conntrack_untracked, cpu).status |= bits;
}
EXPORT_SYMBOL_GPL(nf_ct_untracked_status_or);
static int nf_conntrack_init_init_net(void)
{
int max_factor = 8;
- int ret;
+ int ret, cpu;
/* Idea from tcp.c: use 1/16384 of memory. On i386: 32MB
* machine has 512 buckets. >= 1GB machines have 16384 buckets. */
@@ -1369,10 +1383,12 @@ static int nf_conntrack_init_init_net(void)
goto err_extend;
#endif
/* Set up fake conntrack: to never be deleted, not in any hashes */
-#ifdef CONFIG_NET_NS
- nf_conntrack_untracked.ct_net = &init_net;
-#endif
- atomic_set(&nf_conntrack_untracked.ct_general.use, 1);
+ for_each_possible_cpu(cpu) {
+ struct nf_conn *ct = &per_cpu(nf_conntrack_untracked, cpu);
+
+ write_pnet(&ct->ct_net, &init_net);
+ atomic_set(&ct->ct_general.use, 1);
+ }
/* - and look it like as a confirmed connection */
nf_ct_untracked_status_or(IPS_CONFIRMED | IPS_UNTRACKED);
return 0;
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH][RFC] Infrastructure for out-of-band parameter passing
From: Randy Dunlap @ 2010-06-08 15:31 UTC (permalink / raw)
To: David VomLehn; +Cc: to, netdev
In-Reply-To: <20100608003049.GA29350@dvomlehn-lnx2.corp.sa.net>
On Mon, 7 Jun 2010 17:30:49 -0700 David VomLehn wrote:
> Infrastructure to support out of band/indirect passing of data to functions.
>
> It is useful at times to be able to pass data from one function to another
> nested many stack frames more deeply than the passing function. Doing so
> allows the interfaces in the intervening functions to be simpler, though
> this "hidden" information passing risks increased complexity. In cases
> where this is justified, this simple infrastructure provides the
> functionality.
>
> Out of band data passing is implemented by adding, for each instance,
> an element to the task_struct that serves as the pointer to the top
> of a OOB parameter stack. Data is made available by being pushed
> on the OOB parameter stack by a function, and accessed via the top
> element of the OOB parameter stack.
>
> Signed-off-by: David VomLehn (dvomlehn@cisco.com)
> ---
> include/linux/oobparam.h | 89 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 89 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/oobparam.h b/include/linux/oobparam.h
> new file mode 100644
> index 0000000..6eaa04c
> --- /dev/null
> +++ b/include/linux/oobparam.h
> @@ -0,0 +1,89 @@
...
> +
> +/**
> + * oobparam_push - push an out of band parameter frame on the OOB param stack
> + * @head Pointer to the OOB parameter stack top, which must be in the
> + * task structure.
> + * @frame Pointer to the OOB parameter frame, generally embedded in
> + * another structure
Need a colon ':' after the parameter names for kernel-doc notation:
* @head:
* @frame:
See Documentation/kernel-doc-nano-HOWTO.txt for info or ask me if you have
problems or questions.
Thanks.
> + */
> +static inline void oobparam_push(struct oobparam *top, struct oobparam *frame)
> +{
> + frame->next = top;
> + /* We need to ensure that the pointer in the frame is set prior to
> + * the pointer to the top in case we handle an interrupt in between
> + * the two stores. */
> + wmb();
> + top->next = frame;
> +}
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [PATCH][RFC] Infrastructure for compact call location representation
From: Randy Dunlap @ 2010-06-08 15:44 UTC (permalink / raw)
To: David VomLehn; +Cc: to, netdev
In-Reply-To: <20100608003052.GA29377@dvomlehn-lnx2.corp.sa.net>
On Mon, 7 Jun 2010 17:30:52 -0700 David VomLehn wrote:
> Notes
> o Under simple test conditions, the number of call site IDs allocated
> can be quite small, small enough to fit in 6 bits. That would reduce
> the sk_buff growth to one byte. This is *not* a recommended
> configuration.
> o This is placed in net/core and linux/net since those are the only
> current users, but there is nothing about this that is networking-
> specific.
=> this shouldn't end up in net/
> include/net/callsite-types.h | 160 +++++++++++++++++++
> include/net/callsite.h | 208 +++++++++++++++++++++++++
> net/core/callsite.c | 354 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 722 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/callsite-types.h b/include/net/callsite-types.h
> new file mode 100644
> index 0000000..796cfb1
> --- /dev/null
> +++ b/include/net/callsite-types.h
> @@ -0,0 +1,160 @@
> +#ifndef _LINUX_NET_CALLSITE_TYPES_H_
> +#define _LINUX_NET_CALLSITE_TYPES_H_
> +#include <linux/oobparam.h>
> +
> +/* Pre-defined call site IDs */
> +#define CALLSITE_UNSET 0 /* Never set (default) */
> +#define CALLSITE_UNKNOWN 1 /* Tried to set, but couldn't */
> +#define CALLSITE_START 2 /* First valid call site ID */
> +
> +#define CALLSITE_MAX_ID_SIZE 16 /* Max # bits in a call site ID */
> +
> +/**
> + * struct callsite_id - callsite identifier
> + * @id: Unique value assigned to callsite
> + *
> + * This includes the unique ID assigned to the call site and the information
> + * that defines the location of the call site.
> + */
> +struct callsite_id {
> + unsigned id:CALLSITE_MAX_ID_SIZE;
> +};
> +
> +/* The id value must be set to CALLSITE_UNSET. This is conveniently defined
> + * to have the value zero, so we don't need to explicitly set it */
> +#define CALLSITE_ID_INIT() { \
> + }
> +
> +/**
> + * struct callsite_where - Location information for loaded callsites
> + * @here: Address of code doing the calling (if terse reporting)
> + * @file: Pointer to file name (if not using terse reporting)
> + * @lineno: Line number in file (if not using terse reporting)
> + */
> +struct callsite_where {
> +#ifdef CONFIG_CALLSITE_TERSE
> + void *here; /* Address */
> +#else
> + const char *file; /* Call location */
> + unsigned short lineno;
> +#endif
> +};
> +
> +#ifdef CONFIG_CALLSITE_TERSE
> +#define CALLSITE_WHERE_INIT() { \
> + .here = NULL, \
> + }
> +#else
> +#define CALLSITE_WHERE_INIT() { \
> + .file = __FILE__, \
> + .lineno = __LINE__, \
> + }
> +#endif
> +
> +/**
> + * struct callsite_const - constant per-callsite information
> + * @id: Pointer to the location of the callsite ID
> + * @where: Location information
> + * @module: Pointer to the module om which the callsite exists
> + * @set: Pointer to information that allies to all callsites of this
> + * particular set.
> + */
> +struct callsite_static {
> + struct callsite_id *id;
> + struct callsite_where where;
> + struct module *module;
> + struct callsite_set *set;
> +};
> +
> +#define CALLSITE_STATIC_INIT(_id, _set) { \
> + .id = _id, \
> + .where = CALLSITE_WHERE_INIT(), \
> + .module = THIS_MODULE, \
> + .set = _set, \
> + }
> +
> +/*
> + * callsite_set - information about a set of callsite IDs
> + * @name: Callsite_set name
> + * @width: Number of bits available for callsite ID
> + * Private members:
You can use
* private:
above to keep these parameters from being printed in the kernel-doc output
if that's what you prefer to see.
> + * @warned: Has a warning been printed that no call site ID could
> + * be assigned for this callsite set?
> + * @max_id: The maximim value of a callsite ID. This must fit in
> + * the number of bits allocated to the callsite_id and
> + * must be at least CALLSITE_START.
> + * @next_id: Value of the next callsite ID to give out. Will never
> + * be more than @max_id.
> + * @info: Pointer to callsite_info array.
> + * @lock: Lock that protects the @callsites structure member
> + * @callsite_id_sets: Link to the next callsite_id_set
> + */
> +struct callsite_set {
> + const char *name;
> + unsigned int width;
> + /* private */
> + bool warned:1;
> + unsigned int max_id;
> + unsigned int next_id;
> + struct callsite_info *info;
> + spinlock_t lock;
> + struct list_head callsite_id_sets;
> +};
> +
> +#define CALLSITE_SET_INIT(str_name, _varname, _width) { \
> + .name = str_name, \
> + .width = _width, \
> + .lock = __SPIN_LOCK_UNLOCKED((_varname).lock), \
> + .callsite_id_sets = LIST_HEAD_INIT((_varname).callsite_id_sets), \
> +}
> +
> +/**
> + * struct callsite_frame - data in each "frame" of the callsite stack
> + * @id: Callsite ID
> + * @callsite_oobparam: Data for passing out of band parameters
> + *
> + * Data that is stored on the stack each time a call is made. A linked list
> + * of these is constructed on the stack for each task. In effect, these
> + * are "frames" for the stack of call sites
> + */
> +struct callsite_frame {
> + struct callsite_id id;
> + OOBPARAM_FRAME(frame);
> +};
> +#define CALLSITE_FRAME(name) struct callsite_frame name;
> +
> +/**
> + * struct callsite_top - pointer to the top of the callsite stack
> + * @callsite_top Pointer to the top of the callsite stack
* @callsite_top:
Oh, is it just
* @top:
??
> + */
> +struct callsite_top {
> + OOBPARAM_TOP(top);
> +};
> +#define CALLSITE_TOP(name) struct callsite_top name;
> +#endif
> diff --git a/include/net/callsite.h b/include/net/callsite.h
> new file mode 100644
> index 0000000..a355a23
> --- /dev/null
> +++ b/include/net/callsite.h
> @@ -0,0 +1,208 @@
> +#ifndef _LINUX_NET_CALLSITE_H_
> +#define _LINUX_NET_CALLSITE_H_
> +#include <linux/stringify.h>
> +#include <linux/sched.h>
> +#include <net/callsite-types.h>
> +
> +#ifdef CONFIG_CALLSITE
> +/* CALLSITE_VARS - macro to define all variables local to a call site
> + * @cs_top: Name of a variable in which to store the value of the top
> + * of the stack
> + * @cs_id: Name of the statically allocated variable in which the call
> + * site ID is stored
> + * @cs_static: Name of the statically allocated structure in which constant
> + * data about the call site is stored
> + * @cs_sf: Name of the &struct callsite_frame variable (allocated
> + * on the stack)
> + * @set: Pointer to the &struct callsite_set for this set of call sites
> + * @top: Pointer to the &struct callsite_top for this thread
> + */
> +#define CALLSITE_VARS(cs_top, cs_id, cs_static, cs_sf, set, top) \
> + struct callsite_top *cs_top = (top); \
> + static struct callsite_id cs_id; \
> + static struct callsite_static cs_static = \
> + CALLSITE_STATIC_INIT(&cs_id, (set)); \
> + struct callsite_frame cs_sf
> +
> +/* Define a macro for declaring the variables */
> +#define CALLSITE_DECL(cs_top, cs_id, cs_static, cs_sf, set, top) \
> + CALLSITE_VARS(cs_top, cs_id, cs_static, cs_sf, (set), (top))
> +
> +/**
> + * CALLSITE_CALL - Push a callsite stack "frame" and call a function
> + * @top: Pointer to a pointer to the first in the list of
> + * &callsite_top "frames
> + * @set: Pointer to a &struct callsite_set
> + * @fn: Function returning a non-void value
Is there always an @arg1 ?
> + * @...: Arguments to fn()
> + *
> + * Push a callsite stack "frame" on the stack, call the given function,
> + * and pop the callsite stack frame. Evaluates to the value returned by
> + * the function.
> + */
> +#define CALLSITE_CALL(top, set, fn, arg1, ...) ({ \
> + CALLSITE_DECL(_cs_top, _cs_id, _cs_static, \
> + _cs_stackframe, (set), (top)); \
> + typeof((fn)(arg1, ##__VA_ARGS__)) _cs_result; \
> + callsite_push(_cs_top, &_cs_stackframe, &_cs_id, \
> + &_cs_static); \
> + _cs_result = (fn)(arg1, ##__VA_ARGS__); \
> + callsite_pop(_cs_top); \
> + _cs_result; \
> + })
> +
> +/**
> + * CALLSITE_CALL_VOID - Push a callsite stack "frame" and call a void function
> + * @top: Pointer to a pointer to the first in the list of
> + * callsite_top "frames
> + * @fn: Function of type void
* @arg1:
??
> + * @...: Arguments to fn()
> + *
> + * Push a callsite stack "frame" on the stack, call the given function,
> + * and pop the callsite stack frame.
> + */
> +#define CALLSITE_CALL_VOID(top, set, fn, arg1, ...) do { \
> + CALLSITE_DECL(_cs_top, _cs_id, _cs_static, \
> + _cs_stackframe, (set), (top)); \
> + callsite_push(_cs_top, &_cs_stackframe, &_cs_id, \
> + &_cs_static); \
> + (fn)(arg1, ##__VA_ARGS__); \
> + callsite_pop(_cs_top); \
> + } while (0)
> +
> +#define CALLSITE_CUR(top) \
> + OOBPARAM_CUR(&top->top, struct callsite_frame, frame)
> +
> +extern void callsite_print_where_by_id(struct callsite_set *cs_set,
> + unsigned int id);
> +extern void callsite_assign_id(struct callsite_static *cs_static);
> +extern void callsite_remove_module(struct module *module);
> +extern int callsite_set_register(struct callsite_set *cs_set);
> +
> +/**
> + * callsite_set_id - Set the callsite ID if it isn't already set
> + * @id: Pointer to &callsite_id to check and set
> + * @cs_static: Pointer to &struct callsite_static data for this callsite
> + */
> +static inline void callsite_set_id(struct callsite_id *id,
> + struct callsite_static *cs_static)
> +{
> + if (unlikely(id->id == CALLSITE_UNSET))
> + callsite_assign_id(cs_static);
> +}
...
> +/**
> + * callsite_task_init - initialize a callsite member of the task structure
> + * @p Pointer to the member to initialize
* @p:
> + */
> +static inline void callsite_top_init(struct callsite_top *p)
> +{
> +}
> +#else
> +#define CALLSITE_CALL(top, set, fn, arg1, ...) ({ \
> + (fn)(arg1, ##__VA_ARGS__); \
> + })
...
> diff --git a/net/core/callsite.c b/net/core/callsite.c
> new file mode 100644
> index 0000000..e77d44b
> --- /dev/null
> +++ b/net/core/callsite.c
> @@ -0,0 +1,354 @@
> +/**
> + * struct callsite_info - Per-call site information
> + * @unloaded: Is this in a module that has been unloaded?
> + * @where: Union of location information
> + * @loaded Location information if callsite is loaded
> + * @unloaded Location information if callsite is in an unloaded module
> + * @lock: Lock for updating information for this call site
<lock> is not a struct member below.
> + */
> +struct callsite_info {
> + bool unloaded:1;
> + union {
> + const struct callsite_static *loaded;
> + const char *unloaded;
> + } where;
> +};
...
> +
> +/**
> + * callsite_assign_id - Assign a call site ID
> + * @cs_static: Pointer to static information about the callsite
> + *
> + * If the ID is @CALLSITE_UNSET in a given &struct callsite, this
> + * function is called to assign a call site ID. The value assigned will
> + * normally * be @CALLSITE_START or above, but if we exceed the maximum
> + * size of an ID, * we assign @CALLSITE_UNKNOWN.
stray asterisk? ^
> + */
> +extern void callsite_assign_id(struct callsite_static *cs_static)
> +{
...
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [PATCH][RFC] Infrastructure for compact call location representation
From: Stephen Hemminger @ 2010-06-08 15:44 UTC (permalink / raw)
To: David VomLehn; +Cc: to, netdev
In-Reply-To: <20100608003052.GA29377@dvomlehn-lnx2.corp.sa.net>
On Mon, 7 Jun 2010 17:30:52 -0700
David VomLehn <dvomlehn@cisco.com> wrote:
> This patch allows the location of a call to be recorded as a small integer,
> with each call location ("callsite") assigned a new value the first time
> the code in that location is executed. Locations can be recorded as a
> an address or as a __FILE__/__LINE__ pair. The later is easier to read, but
> requires significantly more space.
>
> The goal here was to record the location in very few bits but, at the same
> time, to have minimal overhead. The key observation towards achieving this
> goals is to note that there are are far fewer locations where calls of
> interest are made than there are addresses. If the site of each call of
> interest is assigned a unique ID, and there are fewer than n of them,
> only log2(n) bits are required to identify the call site. If the IDs
> are assigned dynamically and most call sites aren't reached, you can get
> by with even fewer bits.
>
> This is debugging code and callsite IDs are generally only used when failures
> are detected, so though the mapping from a callsite location to a callsite ID
> must be fast, speed is not as critical for the reverse mapping. Also, an ID
> is assigned to a callsite just once, so it is acceptable to take a while to
> assign an ID, but things should run with minimal delay if an ID is already
> assigned.
>
> The major implementation pieces are:
> 1. Two macros are provided for use in wrapping functions that are to
> be instrumented. CALLSITE_CALL is for functions that return values,
> CALLSITE_CALL_VOID is used for functions that do not.
> 2. The call site infrastructure consists of three data structures:
> a. A statically allocated struct callsite_id holds the ID for
> the call site.
> b. A statically allocated struct callsite_static holds
> information which is constant for each callsite. The call site
> ID could be combined with this, but by separating them I hope
> to avoid polluting the cache with this very cold information.
> c. A struct callsite_frame builds on the oobparam infrastructure
> and holds the call site ID. This is assigned at this time
> if this had not previously been done. This will be pushed on
> the OOB parameter stack before calling the skb_* function
> and popped after it returns.
> 3. A callsite_top structure is added to task_struct. When a call site
> is entered, its callsite_frame is pushed on the call site stack.
> 4. When a function needs to know the call site ID so it can be stored,
> it gets it from the callsite_frame at the top of the call site
> stack.
>
> Notes
> o Under simple test conditions, the number of call site IDs allocated
> can be quite small, small enough to fit in 6 bits. That would reduce
> the sk_buff growth to one byte. This is *not* a recommended
> configuration.
> o This is placed in net/core and linux/net since those are the only
> current users, but there is nothing about this that is networking-
> specific.
>
> Restrictions
> o Call site IDs are never reused, so it is possible to exceed the
> maximum number of IDs by having a large number of call locations.
> In addition, it does not recognize that the same module has been
> unloaded and reloaded, so calls from the reloaded module will be
> assigned new IDs. Detection of incorrect operations on an sk_buff
> is not affected by exhaustion of call site IDs, but it may not be
> possible to determine the location of the last operation.
> CONFIG_DEBUG_SKB_ID_SIZE is set to reduce the sk_buff growth to 16
> bits and should handle most cases. It could be made larger to allow
> more call site IDs, if necessary.
> o The callsite structures for a module will be freed when that module
> is unloaded, even though sk_buffs may be using IDs corresponding to
> those call sites. To allow useful error reporting, the call site
> information in a module being unloaded is copied. If
> CONFIG_CALLSITE_TERSE is not enabled and the module that last changed
> the sk_buff is no longer loaded, the address of the call site
> is no longer valid, so only the function name and offset are printed
> if the module is unloaded. If it is loaded, the address is also
> reported.
>
> History
> v2 Support small callsite IDs and split out out-of-band parameter
> parsing.
> V1 Initial release
>
> Signed-off-by: David VomLehn <dvomlehn@cisco.com>
This is really Linux Kernel Mailing List material (not just netdev). And it will
be a hard sell to get it accepted, because it is basically an alternative call
tracing mechanism, and there are already several of these in use or under development
(see perf and ftrace).
--
^ permalink raw reply
* Re: [PATCHv2 1/2] net: Enable 64-bit net device statistics on 32-bit architectures
From: Ben Hutchings @ 2010-06-08 15:56 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, arnd, netdev, linux-net-drivers
In-Reply-To: <1275996928.14011.83.camel@localhost>
On Tue, 2010-06-08 at 12:35 +0100, Ben Hutchings wrote:
[...]
> > And the whole tree needs to be inspected to make sure there isn't going
> > to be fallout in areas your patch didn't take care of wrt. printf format
> > strings and the like.
> >
> > What was always "unsigned long" is now a variable type, therefore using
> > a fixed printf format string is impossible unless you always cast these
> > things when passed in as printf arguments.
>
> Yes, that's true if there are drivers out there printing members of
> net_device_stats. I admit I haven't checked for that. (Hmm, might be
> time to try Coccinelle.)
There are a few of those, so I'll keep the declared type as unsigned
long rather than making the sizes explicit.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ 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