* Re: [ROSE] Fix dereference of skb pointer after free.
From: David Miller @ 2006-07-04 2:29 UTC (permalink / raw)
To: ralf; +Cc: netdev
In-Reply-To: <20060630133614.GA11735@linux-mips.org>
From: Ralf Baechle <ralf@linux-mips.org>
Date: Fri, 30 Jun 2006 14:36:14 +0100
> If rose_route_frame return success we'll dereference a stale pointer.
> Likely this is only going to result in bad statistics for the ROSE
> interface.
>
> This fixes coverity 946.
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Applied, thanks Ralf.
^ permalink raw reply
* Re: [patch 5/5] Fix a warning in ioatdma
From: David Miller @ 2006-07-04 2:28 UTC (permalink / raw)
To: akpm; +Cc: netdev, bboissin, benoit.boissinot, christopher.leech
In-Reply-To: <200606300927.k5U9RN8m001468@shell0.pdx.osdl.net>
From: akpm@osdl.org
Date: Fri, 30 Jun 2006 02:27:23 -0700
> From: "Benoit Boissinot" <bboissin@gmail.com>
>
> drivers/dma/ioatdma.c: In function 'ioat_init_module':
> drivers/dma/ioatdma.c:830: warning: control reaches end of non-void function
>
> Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
> Cc: "Chris Leech" <christopher.leech@intel.com>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
Applied, thanks a lot.
^ permalink raw reply
* Re: [patch 4/5] drivers/dma/iovlock.c: make num_pages_spanned() static
From: David Miller @ 2006-07-04 2:27 UTC (permalink / raw)
To: akpm; +Cc: netdev, bunk
In-Reply-To: <200606300927.k5U9RMob001465@shell0.pdx.osdl.net>
From: akpm@osdl.org
Date: Fri, 30 Jun 2006 02:27:22 -0700
> From: Adrian Bunk <bunk@stusta.de>
>
> This patch makes the needlessly global num_pages_spanned() static.
>
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
Applied, thanks Adrian.
^ permalink raw reply
* Re: [patch 3/5] af_unix datagram getpeersec fix
From: David Miller @ 2006-07-04 2:26 UTC (permalink / raw)
To: akpm; +Cc: netdev, cxzhang, herbert, jmorris, sds
In-Reply-To: <200606300927.k5U9RLBD001462@shell0.pdx.osdl.net>
From: akpm@osdl.org
Date: Fri, 30 Jun 2006 02:27:21 -0700
> From: Andrew Morton <akpm@osdl.org>
>
> The unix_get_peersec_dgram() stub should have been inlined so that it
> disappears.
>
> Cc: James Morris <jmorris@namei.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Catherine Zhang <cxzhang@watson.ibm.com>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
Applied, thanks Andrew.
^ permalink raw reply
* Re: [patch 2/5] IOAT: fix sparse ulong warning
From: David Miller @ 2006-07-04 2:25 UTC (permalink / raw)
To: akpm; +Cc: netdev, rdunlap, christopher.leech
In-Reply-To: <200606300927.k5U9RK0P001458@shell0.pdx.osdl.net>
From: akpm@osdl.org
Date: Fri, 30 Jun 2006 02:27:20 -0700
> From: Randy Dunlap <rdunlap@xenotime.net>
>
> Fix sparse warning:
> drivers/dma/ioatdma.c:444:32: warning: constant 0xFFFFFFFFFFFFFFC0 is so big it is unsigned long
>
> Also needs a MAINTAINERS entry.
>
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Chris Leech <christopher.leech@intel.com>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
Applied, thanks Randy.
^ permalink raw reply
* Re: [RFC] change netdevice to use struct device instead of struct class_device
From: David Miller @ 2006-07-04 1:57 UTC (permalink / raw)
To: greg; +Cc: jeff, netdev, linux-kernel, akpm
In-Reply-To: <20060703231610.GA18352@kroah.com>
From: Greg KH <greg@kroah.com>
Date: Mon, 3 Jul 2006 16:16:10 -0700
> No, not really. According to Documentation/ABI/testing/sysfs-class all
> code that uses /sys/class/foo/ needs to be able to handle the fact that
> those entries might be symlinks and not just directories. Everything
> that I know of already works properly because the input layer has had
> symlinks in /sys/class/input for quite some time now.
>
> Do you know of any tools that use /sys/class/net/ that can not handle
> symlinks there? I've been running this on my boxes for about a week now
> with no noticeable issues. Renaming interfaces works just fine too.
I do not think this change will cause any problems.
^ permalink raw reply
* Get the freshest Now you have chance to do it Feel Pleasure from
From: Hubert @ 2006-07-04 1:55 UTC (permalink / raw)
To: netdev
Hello to you!
Have you ever wished to have more intense final?
But some of us think it’s impossible
Have you some doubt?
Just take a look: http://www.basszass.com
The prices are really low and the quality it truly very high!
^ permalink raw reply
* Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats
From: Andrew Morton @ 2006-07-04 1:01 UTC (permalink / raw)
To: Shailabh Nagar
Cc: hadi, pj, Valdis.Kletnieks, jlan, balbir, csturtiv, linux-kernel,
netdev
In-Reply-To: <44A9BC4D.7030803@watson.ibm.com>
On Mon, 03 Jul 2006 20:54:37 -0400
Shailabh Nagar <nagar@watson.ibm.com> wrote:
> > What happens when a listener exits without doing deregistration
> > (or if the listener attempts to register another cpumask while a current
> > registration is still active).
> >
> ( Jamal, your thoughts on this problem would be appreciated)
>
> Problem is that we have a listener task which has "registered" with
> taskstats and caused
> its pid to be stored in various per-cpu lists of listeners. Later, when
> some other task exits on a given cpu, its exit data is sent using
> genlmsg_unicast on each pid present on that cpu's list.
>
> If the listener exits without doing a "deregister", its pid continues to
> be kept around, obviously not a good thing. So we need some way of
> detecting the situation (task is no longer listening on
> these cpus events) that is efficient.
Also need to address the case where the listener has closed off his file
descriptor but continues to run.
So hooking into listener's exit() isn't appropriate - the teardown is
associated with the lifetime of the fd, not of the process. If we do that,
exit() gets handled for free.
^ permalink raw reply
* Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats
From: Shailabh Nagar @ 2006-07-04 0:54 UTC (permalink / raw)
To: hadi
Cc: Andrew Morton, pj, Valdis.Kletnieks, jlan, balbir, csturtiv,
linux-kernel, netdev
In-Reply-To: <44A9881F.7030103@watson.ibm.com>
Shailabh Nagar wrote:
> Andrew Morton wrote:
>
>> On Fri, 30 Jun 2006 23:37:10 -0400
>> Shailabh Nagar <nagar@watson.ibm.com> wrote:
>>
>>
>>
>>>> Set aside the implementation details and ask "what is a good design"?
>>>>
>>>> A kernel-wide constant, whether determined at build-time or by a
>>>> /proc poke
>>>> isn't a nice design.
>>>>
>>>> Can we permit userspace to send in a netlink message describing a
>>>> cpumask? That's back-compatible.
>>>>
>>>>
>>>>
>>>
>>> Yes, that should be doable. And passing in a cpumask is much better
>>> since we no longer
>>> have to maintain mappings.
>>>
>>> So the strawman is:
>>> Listener bind()s to genetlink using its real pid.
>>> Sends a separate "registration" message with cpumask to listen to.
>>> Kernel stores (real) pid and cpumask.
>>> During task exit, kernel goes through each registered listener
>>> (small list) and decides which
>>> one needs to get this exit data and calls a genetlink_unicast to
>>> each one that does need it.
>>>
>>> If number of listeners is small, the lookups should be swift enough.
>>> If it grows large, we
>>> can consider a fancier lookup (but there I go again, delving into
>>> implementation too early :-)
>>>
>>
>>
>> We'll need a map.
>>
>> 1024 CPUs, 1024 listeners, 1000 exits/sec/CPU and we're up to a million
>> operations per second per CPU. Meltdown.
>>
>> But it's a pretty simple map. A per-cpu array of pointers to the
>> head of a
>> linked list. One lock for each CPU's list.
>>
>>
> Here's a patch that implements the above ideas.
>
> A listener register's interest by specifying a cpumask in the
> cpulist format (comma separated ranges of cpus). The listener's pid
> is entered into per-cpu lists for those cpus and exit events from those
> cpus go to the listeners using netlink unicasts.
>
> Please comment.
>
> Andrew, this is not being proposed for inclusion yet since there is
> atleast one more issue that needs to be resolved:
>
> What happens when a listener exits without doing deregistration
> (or if the listener attempts to register another cpumask while a current
> registration is still active).
>
( Jamal, your thoughts on this problem would be appreciated)
Problem is that we have a listener task which has "registered" with
taskstats and caused
its pid to be stored in various per-cpu lists of listeners. Later, when
some other task exits on a given cpu, its exit data is sent using
genlmsg_unicast on each pid present on that cpu's list.
If the listener exits without doing a "deregister", its pid continues to
be kept around, obviously not a good thing. So we need some way of
detecting the situation (task is no longer listening on
these cpus events) that is efficient.
Two solutions come to mind:
1. During the exit of every task check to see if it is is already
"registered" with taskstats. If so, do a cleanup of its pid on various
per-cpu lists.
2. Before doing a genlmsg_unicast to a pid on one of the per-cpu lists
(or if genlmsg_unicast
fails with a -ECONNREFUSED, a result of netlink_lookup failing for that
pid), then just delete
it from that cpu's list and continue.
1 is more desirable because its the right place to catch this and
happens relatively rarely
(few listener exits compared to all exits). However, how can we check
whether a task/pid
has registered with taskstats earlier ? Again, two possibilities
- Maintain a list of registered listeners within taskstats and check that.
- try to leverage netlink's nl_pid_hash which maintains the same kind of
info for each protocol.
Thus a netlink_lookup of the pid would save a lot of work.
However, the netlink layer's hashtable appears to be for the entire
NETLINK_GENERIC
protocol and not just for the taskstats client of NETLINK_GENERIC. So
even if a task has
deregistered with taskstats, as long as it has some other
NETLINK_GENERIC socket open,
it will still show up as "connected" as far as netlink is concerned.
Jamal - is my interpretation correct ? Do I need to essentially
replicate the pidhash at the
taskstats layer ? Thoughts on whether there's any way genetlink can
provide support for this or
whether its desirable etc. (we appear to be the second user of genetlink
- this may not be a
common need going forward).
1 has the disadvantage that if such a situation is detected, one has to
iterate over all cpus in
the system, deleting that pid from any per-cpu list it happens to be in.
One could store the cpumask that the listener originally used to
optimize this search. usual tradeoff of storage vs. time.
2 avoids the problem just mentioned since it delegates the task of
cleanup to each cpu at the cost
of incurring an extra check for each listener for each exit on that cpu.
By storing the task_struct instead of the pid in the per-cpu lists, the
check can be made quite
cheap.
But one problem with 2 is the issue of recycled task_structs and pids.
Since the stale task on the
per-cpu listener list could have exited a while back, its possible its
alive at the time of the check
and has even registered with a different interest list ! So it'll
receive events it didn't register for.
I guess this again calls for us to maintain the listener list within
taskstats explicitly (solution 1)
and explicitly catch the exit of the task/pid.
Thoughts ?
--Shailabh
^ permalink raw reply
* Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats
From: Andrew Morton @ 2006-07-04 0:38 UTC (permalink / raw)
To: Shailabh Nagar
Cc: pj, Valdis.Kletnieks, jlan, balbir, csturtiv, linux-kernel, hadi,
netdev
In-Reply-To: <44A9B2B0.3000205@watson.ibm.com>
On Mon, 03 Jul 2006 20:13:36 -0400
Shailabh Nagar <nagar@watson.ibm.com> wrote:
> >>+ if (!s)
> >>+ return -ENOMEM;
> >>+ s->pid = pid;
> >>+ INIT_LIST_HEAD(&s->list);
> >>+
> >>+ down_write(sem);
> >>+ list_add(&s->list, head);
> >>+ up_write(sem);
> >>+
> >>+ if (cpu == mycpu)
> >>+ preempt_enable();
> >>
> >>
> >
> >Actually, I don't understand the tricks which are going on with the local CPU here.
> >What's it all for?
> >
> >
> I was wanting to do a get_cpu_var for listener_list & sem
> for the current cpu and per_cpu otherwise (since thats what I thought
> was the recommendation
> for accessing the local cpu's variable). Perhaps the preempt_disable is
> uncalled for ?
Well we have a problem. You want to grab this CPU's list, and then lock a
semaphore. But taking a semaphore is a sleeping operation.
Fortunately, there's really no need to stay on-CPU at all. When userspace
is setting or clearing entries in the map, userspace _told_ us which CPU to
manipulate, so this code can be running on any CPU at all. So just go grab
the Nth entry in the array and acquire the lock.
And when the time comes to send some statistics, just use
raw_smp_processor_id() and don't use preempt_disable() at all. If we end
up hopping over to another CPU, well at least we tried. All we can do here
is to run raw_smp_processor_id() as early as possible to reduce the
possibility that we'll get a different CPU from the one which this task
really exited on.
IOW: in all cases we were provided with explicit CPU numbers from other
sources. So no preemption disabling is required.
^ permalink raw reply
* Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats
From: Shailabh Nagar @ 2006-07-04 0:13 UTC (permalink / raw)
To: Andrew Morton
Cc: pj, Valdis.Kletnieks, jlan, balbir, csturtiv, linux-kernel, hadi,
netdev
In-Reply-To: <20060703144106.cc5bd6f6.akpm@osdl.org>
Andrew Morton wrote:
>On Mon, 03 Jul 2006 17:11:59 -0400
>Shailabh Nagar <nagar@watson.ibm.com> wrote:
>
>
>>
>> static inline void taskstats_exit_alloc(struct taskstats **ptidstats)
>> {
>> *ptidstats = NULL;
>>- if (taskstats_has_listeners())
>>+ if (!list_empty(&get_cpu_var(listener_list)))
>> *ptidstats = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL);
>>+ put_cpu_var(listener_list);
>> }
>>
>>
>
>It's time to uninline this function..
>
>
>
>> static inline void taskstats_exit_free(struct taskstats *tidstats)
>>Index: linux-2.6.17-mm3equiv/kernel/taskstats.c
>>===================================================================
>>--- linux-2.6.17-mm3equiv.orig/kernel/taskstats.c 2006-06-30 23:38:39.000000000 -0400
>>+++ linux-2.6.17-mm3equiv/kernel/taskstats.c 2006-07-02 00:16:18.000000000 -0400
>>@@ -19,6 +19,8 @@
>> #include <linux/kernel.h>
>> #include <linux/taskstats_kern.h>
>> #include <linux/delayacct.h>
>>+#include <linux/cpumask.h>
>>+#include <linux/percpu.h>
>> #include <net/genetlink.h>
>> #include <asm/atomic.h>
>>
>>@@ -26,6 +28,9 @@ static DEFINE_PER_CPU(__u32, taskstats_s
>> static int family_registered = 0;
>> kmem_cache_t *taskstats_cache;
>>
>>+DEFINE_PER_CPU(struct list_head, listener_list);
>>+static DEFINE_PER_CPU(struct rw_semaphore, listener_list_sem);
>>
>>
>
>Which will permit listener_list to become static - it wasn't a good name
>for a global anyway.
>
>I suggest you implement a new
>
>struct whatever {
> struct rw_semaphore sem;
> struct list_head list;
>};
>
>
Ok. The listener_list was a global to allow taskstats_exit_alloc to
access but this is better.
>static DEFINE_PER_CPU(struct whatever, listener_aray);
>
>
>
>
>> static int prepare_reply(struct genl_info *info, u8 cmd, struct sk_buff **skbp,
>> void **replyp, size_t size)
>> {
>>@@ -77,6 +92,8 @@ static int prepare_reply(struct genl_inf
>> static int send_reply(struct sk_buff *skb, pid_t pid, int event)
>> {
>> struct genlmsghdr *genlhdr = nlmsg_data((struct nlmsghdr *)skb->data);
>>+ struct rw_semaphore *sem;
>>+ struct list_head *p, *head;
>> void *reply;
>> int rc;
>>
>>@@ -88,9 +105,30 @@ static int send_reply(struct sk_buff *sk
>> return rc;
>> }
>>
>>- if (event == TASKSTATS_MSG_MULTICAST)
>>- return genlmsg_multicast(skb, pid, TASKSTATS_LISTEN_GROUP);
>>- return genlmsg_unicast(skb, pid);
>>+ if (event == TASKSTATS_MSG_UNICAST)
>>+ return genlmsg_unicast(skb, pid);
>>+
>>+ /*
>>+ * Taskstats multicast is unicasts to listeners who have registered
>>+ * interest in this cpu
>>+ */
>>+ sem = &get_cpu_var(listener_list_sem);
>>+ head = &get_cpu_var(listener_list);
>>
>>
>
>This has a double preempt_disable(), but the above will fix that.
>
>
>
>>+ down_read(sem);
>>+ list_for_each(p, head) {
>>+ int ret;
>>+ struct listener *s = list_entry(p, struct listener, list);
>>+ ret = genlmsg_unicast(skb, s->pid);
>>+ if (ret)
>>+ rc = ret;
>>+ }
>>+ up_read(sem);
>>+
>>+ put_cpu_var(listener_list);
>>+ put_cpu_var(listener_list_sem);
>>+
>>+ return rc;
>> }
>>
>> static int fill_pid(pid_t pid, struct task_struct *pidtsk,
>>@@ -201,8 +239,73 @@ ret:
>> return;
>> }
>>
>>+static int add_del_listener(pid_t pid, cpumask_t *maskp, int isadd)
>>+{
>>+ struct listener *s;
>>+ unsigned int cpu, mycpu;
>>+ cpumask_t mask;
>>+ struct rw_semaphore *sem;
>>+ struct list_head *head, *p;
>>
>>-static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
>>+ memcpy(&mask, maskp, sizeof(cpumask_t));
>>+ if (cpus_empty(mask))
>>+ return -EINVAL;
>>+
>>+ mycpu = get_cpu();
>>+ put_cpu();
>>
>>
>
>This is effectively raw_smp_processor_id(). And after the put_cpu(),
>`mycpu' is meaningless.
>
>
Hmm.
>
>
>>+ if (isadd == REGISTER) {
>>+ for_each_cpu_mask(cpu, mask) {
>>+ if (!cpu_possible(cpu))
>>+ continue;
>>+ if (cpu == mycpu)
>>+ preempt_disable();
>>+
>>+ sem = &per_cpu(listener_list_sem, cpu);
>>+ head = &per_cpu(listener_list, cpu);
>>+
>>+ s = kmalloc(sizeof(struct listener), GFP_KERNEL);
>>
>>
>
>Cannot do GFP_KERNEL inside preempt_disable().
>
>There's no easy solution to this problem. GFP_ATOMIC is not a good fix at
>all. One approach would be to run lock_cpu_hotplug(), then allocate (with
>GFP_KERNEL) all the memory which will be needed within the locked region,
>then take the lock, then use that preallocated memory.
>
>
>You should use kmalloc_node() here, to ensure that the memory on each CPU's
>list resides with that CPU's local memory (not _this_ CPU's local memory).
>
>
Ok.
>
>
>>+ if (!s)
>>+ return -ENOMEM;
>>+ s->pid = pid;
>>+ INIT_LIST_HEAD(&s->list);
>>+
>>+ down_write(sem);
>>+ list_add(&s->list, head);
>>+ up_write(sem);
>>+
>>+ if (cpu == mycpu)
>>+ preempt_enable();
>>
>>
>
>Actually, I don't understand the tricks which are going on with the local CPU here.
>What's it all for?
>
>
I was wanting to do a get_cpu_var for listener_list & sem
for the current cpu and per_cpu otherwise (since thats what I thought
was the recommendation
for accessing the local cpu's variable). Perhaps the preempt_disable is
uncalled for ?
>
>
>
>>+ }
>>+ } else {
>>+ for_each_cpu_mask(cpu, mask) {
>>+ struct list_head *tmp;
>>+
>>+ if (!cpu_possible(cpu))
>>+ continue;
>>
>>
>
>I guess you could just do cpus_and(mask, cpus_possible_map) on entry.
>
>
Yup !
>
>
>
>>+ if (cpu == mycpu)
>>+ preempt_disable();
>>+
>>+ sem = &per_cpu(listener_list_sem, cpu);
>>+ head = &per_cpu(listener_list, cpu);
>>+
>>+ down_write(sem);
>>+ list_for_each_safe(p, tmp, head) {
>>+ s = list_entry(p, struct listener, list);
>>+ if (s->pid == pid) {
>>+ list_del(&s->list);
>>
>>
>
>kfree(s);
>
>
Oops.
>
>
>>+ break;
>>+ }
>>+ }
>>+ up_write(sem);
>>+
>>+ if (cpu == mycpu)
>>+ preempt_enable();
>>+ }
>>+ }
>>+ return 0;
>>+}
>>+
>>+static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
>> {
>> int rc = 0;
>> struct sk_buff *rep_skb;
>>@@ -210,6 +313,21 @@ static int taskstats_send_stats(struct s
>> void *reply;
>> size_t size;
>> struct nlattr *na;
>>+ cpumask_t mask;
>>+
>>+ if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) {
>>+ na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
>>+ cpulist_parse((char *)nla_data(na), mask);
>>
>>
>
>OK, so we're passing in an ASCII string. Fair enough, I think. Paul would
>know better.
>
>
^ permalink raw reply
* Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats
From: Shailabh Nagar @ 2006-07-04 0:09 UTC (permalink / raw)
To: Paul Jackson
Cc: akpm, Valdis.Kletnieks, jlan, balbir, csturtiv, linux-kernel,
hadi, netdev
In-Reply-To: <20060703093148.5e61a7e4.pj@sgi.com>
Paul Jackson wrote:
>Shailabh wrote:
>
>
>>I don't know if there are buffer overflow
>>issues in passing a string
>>
>>
>
>I don't know if this comment applies to "the standard netlink way of
>passing it up using NLA_STRING", but the way I deal with buffer length
>issues in the cpuset code is to insist that the user code express the
>list in no fewer than 100 + 6 * NR_CPUS bytes:
>
>From kernel/cpuset.c:
>
> /* Crude upper limit on largest legitimate cpulist user might write. */
> if (nbytes > 100 + 6 * NR_CPUS)
> return -E2BIG;
>
>This lets the user specify the buffer size passed in, but prevents
>them from trying a denial of service attack on the kernel by trying
>to pass in a huge buffer.
>
>If the user can't figure out how to write the desired cpulist in
>that size, then tough toenails.
>
>
Paul,
Perhaps I should use the the other ascii format for specifying cpumasks
since its more amenable
to specifying an upper bound for the length of the ascii string and is
more compact ?
That format (the one used in lib/bitmap.c:bitmap_parse) is comma
separated chunks of hex digits
with each chunk specifying 32 bits of the desired cpumask.
So
((NR_CPUS + 32) / 32) * 8 + 1
(8 hex characters for each 32 cpus, and 1 extra character for null
terminator)
would be an upper bound that would accomodate all the cpus for sure.
Thoughts ?
--Shailabh
--Shailabh
^ permalink raw reply
* Re: [RFC] change netdevice to use struct device instead of struct class_device
From: Greg KH @ 2006-07-03 23:16 UTC (permalink / raw)
To: Jeff Garzik; +Cc: netdev, linux-kernel, Andrew Morton
In-Reply-To: <44A9A345.8040706@garzik.org>
On Mon, Jul 03, 2006 at 07:07:49PM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> >I have a patch here that converts the network device structure to use
> >the struct device instead of struct class_device structure. It's a bit
> >too big to post here, so it's at:
> > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/network-class_device-to-device.patch
>
> So.... this is a userspace ABI change, then?
No, not really. According to Documentation/ABI/testing/sysfs-class all
code that uses /sys/class/foo/ needs to be able to handle the fact that
those entries might be symlinks and not just directories. Everything
that I know of already works properly because the input layer has had
symlinks in /sys/class/input for quite some time now.
Do you know of any tools that use /sys/class/net/ that can not handle
symlinks there? I've been running this on my boxes for about a week now
with no noticeable issues. Renaming interfaces works just fine too.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC] change netdevice to use struct device instead of struct class_device
From: Jeff Garzik @ 2006-07-03 23:07 UTC (permalink / raw)
To: Greg KH; +Cc: netdev, linux-kernel, Andrew Morton
In-Reply-To: <20060703224719.GA14176@kroah.com>
Greg KH wrote:
> I have a patch here that converts the network device structure to use
> the struct device instead of struct class_device structure. It's a bit
> too big to post here, so it's at:
> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/network-class_device-to-device.patch
So.... this is a userspace ABI change, then?
Jeff
^ permalink raw reply
* [RFC] change netdevice to use struct device instead of struct class_device
From: Greg KH @ 2006-07-03 22:47 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel
I have a patch here that converts the network device structure to use
the struct device instead of struct class_device structure. It's a bit
too big to post here, so it's at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/network-class_device-to-device.patch
I can split it out, but then it will not build for the intermediate
steps, which 'git bisect' users might not appreciate. If you all want
me to break it up to make it easier to review, please let me know and
I'll be glad to do it.
With this patch applied, sysfs now looks like:
$ tree /sys/class/net/
/sys/class/net/
|-- eth0 -> ../../devices/pci0000:00/0000:00:02.0/0000:01:00.2/0000:03:0e.0/eth0
|-- gerg -> ../../devices/pci0000:00/0000:00:02.0/0000:01:00.2/0000:03:0c.0/gerg
`-- lo -> ../../devices/lo
Instead of the different directories being in /sys/class/net.
What this buys us is now the different network devices can be called by
the core when the system is shutting down or restoring, with the
suspend/resume changes that Linus has written (and are now in -mm).
This can be used by the network core to stop the queue, or whatever else
it desires.
Other good things happen with this, as the network devices are now real
devices, instead of the second-class citizens that "class_device" was.
(which is one reason why I'm getting rid of class_device entirely).
The patch needs some other changes to the driver core that are also in
my git tree, and included in the -mm release. Specifically these
patches are needed:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/device-groups.patch
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/device-class-parent.patch
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/device-class-attr.patch
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/device_rename.patch
And if you are curious, the suspend stuff from Linus is at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/suspend-infrastructure-cleanup-and-extension.patch
I can gladly keep this in my tree (due to the previously mentioned
requirements) and eventually merge it with Linus after 2.6.18 is out, if
no one objects to it.
thanks,
greg k-h
p.s. That bonding code! WTF is going on with poking around in the
internals of krefs? And why are you returning more than one value
from a sysfs file? I thought I asked that this stuff be fixed up a
long time ago?
^ permalink raw reply
* Re: [patch 3/4] myri10ge - Use dev_info() when printing parameters after probe
From: Brice Goglin @ 2006-07-03 22:44 UTC (permalink / raw)
To: netdev; +Cc: Jeff Garzik
In-Reply-To: <20060703220517.236511000@loulous.org>
Please forget this one, something went wrong, it contains both #3 and
#4. I have resent #3 and #4 separately.
Brice
brice@myri.com wrote:
> Displaying the interface name when listing the device parameters
> at the end of myri10ge_probe is not a good idea since udev might
> rename the interface soon afterwards.
> Print the bus id instead, using dev_info().
>
> Signed-off-by: Brice Goglin <brice@myri.com>
> ---
> drivers/net/myri10ge/myri10ge.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> Index: linux-mm/drivers/net/myri10ge/myri10ge.c
> ===================================================================
> --- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:02:52.000000000 -0400
> +++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:04:15.000000000 -0400
> @@ -2734,11 +2734,10 @@
> dev_err(&pdev->dev, "register_netdev failed: %d\n", status);
> goto abort_with_irq;
> }
> -
> - printk(KERN_INFO "myri10ge: %s: %s IRQ %d, tx bndry %d, fw %s, WC %s\n",
> - netdev->name, (mgp->msi_enabled ? "MSI" : "xPIC"),
> - pdev->irq, mgp->tx.boundary, mgp->fw_name,
> - (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
> + dev_info(dev, "%s IRQ %d, tx bndry %d, fw %s, WC %s\n",
> + (mgp->msi_enabled ? "MSI" : "xPIC"),
> + pdev->irq, mgp->tx.boundary, mgp->fw_name,
> + (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
>
> return 0;
>
> >From bgoglin@loulous.org Mon Jul 3 18:05:17 2006
> Message-Id: <20060703220517.426089000@loulous.org>
> References: <20060703220230.606593000@myri.com>
> User-Agent: quilt/0.45-1
> Date: Mon, 03 Jul 2006 18:02:34 -0400
> From: brice@myri.com
> To: netdev@vger.kernel.org
> Cc: Brice Goglin <brice@myri.com>
> Subject: [patch 4/4] myri10ge - Export more parameters to ethtool.
> Content-Disposition: inline; filename=myri10ge4-export_more_parameters_to_ethtool.patch
>
> Add the IRQ line, the tx_boundary, and whether Write-combining and MSI
> are enabled to the list of parameters that are exported to ethtool.
>
> Signed-off-by: Brice Goglin <brice@myri.com>
> ---
> drivers/net/myri10ge/myri10ge.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> Index: linux-mm/drivers/net/myri10ge/myri10ge.c
> ===================================================================
> --- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:04:47.000000000 -0400
> +++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:39:29.000000000 -0400
> @@ -1288,6 +1288,7 @@
> "tx_aborted_errors", "tx_carrier_errors", "tx_fifo_errors",
> "tx_heartbeat_errors", "tx_window_errors",
> /* device-specific stats */
> + "tx_boundary", "WC", "irq", "MSI",
> "read_dma_bw_MBs", "write_dma_bw_MBs", "read_write_dma_bw_MBs",
> "serial_number", "tx_pkt_start", "tx_pkt_done",
> "tx_req", "tx_done", "rx_small_cnt", "rx_big_cnt",
> @@ -1326,6 +1327,10 @@
> for (i = 0; i < MYRI10GE_NET_STATS_LEN; i++)
> data[i] = ((unsigned long *)&mgp->stats)[i];
>
> + data[i++] = (unsigned int)mgp->tx.boundary;
> + data[i++] = (unsigned int)(mgp->mtrr >= 0);
> + data[i++] = (unsigned int)mgp->pdev->irq;
> + data[i++] = (unsigned int)mgp->msi_enabled;
> data[i++] = (unsigned int)mgp->read_dma;
> data[i++] = (unsigned int)mgp->write_dma;
> data[i++] = (unsigned int)mgp->read_write_dma;
>
>
^ permalink raw reply
* [patch 3/4] myri10ge - Use dev_info() when printing parameters after probe
From: Brice Goglin @ 2006-07-03 22:41 UTC (permalink / raw)
To: netdev; +Cc: brice
In-Reply-To: <20060703220230.606593000@myri.com>
Displaying the interface name when listing the device parameters
at the end of myri10ge_probe is not a good idea since udev might
rename the interface soon afterwards.
Print the bus id instead, using dev_info().
Signed-off-by: Brice Goglin <brice@myri.com>
---
drivers/net/myri10ge/myri10ge.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:02:52.000000000 -0400
+++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:04:15.000000000 -0400
@@ -2734,11 +2734,10 @@
dev_err(&pdev->dev, "register_netdev failed: %d\n", status);
goto abort_with_irq;
}
-
- printk(KERN_INFO "myri10ge: %s: %s IRQ %d, tx bndry %d, fw %s, WC %s\n",
- netdev->name, (mgp->msi_enabled ? "MSI" : "xPIC"),
- pdev->irq, mgp->tx.boundary, mgp->fw_name,
- (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
+ dev_info(dev, "%s IRQ %d, tx bndry %d, fw %s, WC %s\n",
+ (mgp->msi_enabled ? "MSI" : "xPIC"),
+ pdev->irq, mgp->tx.boundary, mgp->fw_name,
+ (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
return 0;
^ permalink raw reply
* Re: [PATCH 38 of 39] IB/ipath - More changes to support InfiniPath on PowerPC 970 systems
From: Anton Blanchard @ 2006-07-03 22:25 UTC (permalink / raw)
To: David Miller
Cc: bos, akpm, rdreier, mst, openib-general, linux-kernel, netdev,
matthew
In-Reply-To: <20060629.150417.78710870.davem@davemloft.net>
Hi,
> Please fix the generic code if it doesn't provide the facility
> you need at the moment. Don't shoe horn it into your driver
> just to make up for that.
Ive had 3 drivers asking for write combining recently so I agree this is
a good idea. How about ioremap_wc as suggested by Willy:
http://marc.theaimsgroup.com/?l=linux-kernel&m=114374741828040&w=2
Anton
^ permalink raw reply
* [patch 4/4] myri10ge - Export more parameters to ethtool
From: Brice Goglin @ 2006-07-03 22:16 UTC (permalink / raw)
To: netdev; +Cc: brice
In-Reply-To: <20060703220230.606593000@myri.com>
Add the IRQ line, the tx_boundary, and whether Write-combining and MSI
are enabled to the list of parameters that are exported to ethtool.
Signed-off-by: Brice Goglin <brice@myri.com>
---
drivers/net/myri10ge/myri10ge.c | 5 +++++
1 file changed, 5 insertions(+)
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:04:47.000000000 -0400
+++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:39:29.000000000 -0400
@@ -1288,6 +1288,7 @@
"tx_aborted_errors", "tx_carrier_errors", "tx_fifo_errors",
"tx_heartbeat_errors", "tx_window_errors",
/* device-specific stats */
+ "tx_boundary", "WC", "irq", "MSI",
"read_dma_bw_MBs", "write_dma_bw_MBs", "read_write_dma_bw_MBs",
"serial_number", "tx_pkt_start", "tx_pkt_done",
"tx_req", "tx_done", "rx_small_cnt", "rx_big_cnt",
@@ -1326,6 +1327,10 @@
for (i = 0; i < MYRI10GE_NET_STATS_LEN; i++)
data[i] = ((unsigned long *)&mgp->stats)[i];
+ data[i++] = (unsigned int)mgp->tx.boundary;
+ data[i++] = (unsigned int)(mgp->mtrr >= 0);
+ data[i++] = (unsigned int)mgp->pdev->irq;
+ data[i++] = (unsigned int)mgp->msi_enabled;
data[i++] = (unsigned int)mgp->read_dma;
data[i++] = (unsigned int)mgp->write_dma;
data[i++] = (unsigned int)mgp->read_write_dma;
^ permalink raw reply
* [patch 1/4] myri10ge - Drop unused pm_state
From: brice @ 2006-07-03 22:02 UTC (permalink / raw)
To: netdev; +Cc: Brice Goglin
In-Reply-To: <20060703220230.606593000@myri.com>
[-- Attachment #1: myri10ge1-drop_pm_state.patch --]
[-- Type: text/plain, Size: 634 bytes --]
The pm_state field in the myri10ge_priv structure is unused. Drop it.
Signed-off-by: Brice Goglin <brice@myri.com>
---
drivers/net/myri10ge/myri10ge.c | 1 -
1 file changed, 1 deletion(-)
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:00:22.000000000 -0400
+++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:00:27.000000000 -0400
@@ -188,7 +188,6 @@
int vendor_specific_offset;
u32 devctl;
u16 msi_flags;
- u32 pm_state[16];
u32 read_dma;
u32 write_dma;
u32 read_write_dma;
^ permalink raw reply
* [patch 2/4] myri10ge - Drop ununsed nvidia chipset id
From: brice @ 2006-07-03 22:02 UTC (permalink / raw)
To: netdev; +Cc: Brice Goglin
In-Reply-To: <20060703220230.606593000@myri.com>
[-- Attachment #1: myri10ge2-drop_ck804_pci_id.patch --]
[-- Type: text/plain, Size: 801 bytes --]
The workaround for the AER capability of the nVidia chipset has been
removed, we don't need this PCI id anymore. Drop it.
Signed-off-by: Brice Goglin <brice@myri.com>
---
drivers/net/myri10ge/myri10ge.c | 2 --
1 file changed, 2 deletions(-)
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:01:33.000000000 -0400
+++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:01:39.000000000 -0400
@@ -2196,8 +2196,6 @@
* any other device, except if forced with myri10ge_ecrc_enable > 1.
*/
-#define PCI_DEVICE_ID_NVIDIA_NFORCE_CK804_PCIE 0x005d
-
static void myri10ge_enable_ecrc(struct myri10ge_priv *mgp)
{
struct pci_dev *bridge = mgp->pdev->bus->self;
^ permalink raw reply
* [patch 3/4] myri10ge - Use dev_info() when printing parameters after probe
From: brice @ 2006-07-03 22:02 UTC (permalink / raw)
To: netdev; +Cc: Brice Goglin
In-Reply-To: <20060703220230.606593000@myri.com>
[-- Attachment #1: myri10ge3-do_not_print_interface_name_at_the_end_of_probe.patch --]
[-- Type: text/plain, Size: 3020 bytes --]
Displaying the interface name when listing the device parameters
at the end of myri10ge_probe is not a good idea since udev might
rename the interface soon afterwards.
Print the bus id instead, using dev_info().
Signed-off-by: Brice Goglin <brice@myri.com>
---
drivers/net/myri10ge/myri10ge.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:02:52.000000000 -0400
+++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:04:15.000000000 -0400
@@ -2734,11 +2734,10 @@
dev_err(&pdev->dev, "register_netdev failed: %d\n", status);
goto abort_with_irq;
}
-
- printk(KERN_INFO "myri10ge: %s: %s IRQ %d, tx bndry %d, fw %s, WC %s\n",
- netdev->name, (mgp->msi_enabled ? "MSI" : "xPIC"),
- pdev->irq, mgp->tx.boundary, mgp->fw_name,
- (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
+ dev_info(dev, "%s IRQ %d, tx bndry %d, fw %s, WC %s\n",
+ (mgp->msi_enabled ? "MSI" : "xPIC"),
+ pdev->irq, mgp->tx.boundary, mgp->fw_name,
+ (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
return 0;
>From bgoglin@loulous.org Mon Jul 3 18:05:17 2006
Message-Id: <20060703220517.426089000@loulous.org>
References: <20060703220230.606593000@myri.com>
User-Agent: quilt/0.45-1
Date: Mon, 03 Jul 2006 18:02:34 -0400
From: brice@myri.com
To: netdev@vger.kernel.org
Cc: Brice Goglin <brice@myri.com>
Subject: [patch 4/4] myri10ge - Export more parameters to ethtool.
Content-Disposition: inline; filename=myri10ge4-export_more_parameters_to_ethtool.patch
Add the IRQ line, the tx_boundary, and whether Write-combining and MSI
are enabled to the list of parameters that are exported to ethtool.
Signed-off-by: Brice Goglin <brice@myri.com>
---
drivers/net/myri10ge/myri10ge.c | 5 +++++
1 file changed, 5 insertions(+)
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-mm.orig/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:04:47.000000000 -0400
+++ linux-mm/drivers/net/myri10ge/myri10ge.c 2006-07-03 16:39:29.000000000 -0400
@@ -1288,6 +1288,7 @@
"tx_aborted_errors", "tx_carrier_errors", "tx_fifo_errors",
"tx_heartbeat_errors", "tx_window_errors",
/* device-specific stats */
+ "tx_boundary", "WC", "irq", "MSI",
"read_dma_bw_MBs", "write_dma_bw_MBs", "read_write_dma_bw_MBs",
"serial_number", "tx_pkt_start", "tx_pkt_done",
"tx_req", "tx_done", "rx_small_cnt", "rx_big_cnt",
@@ -1326,6 +1327,10 @@
for (i = 0; i < MYRI10GE_NET_STATS_LEN; i++)
data[i] = ((unsigned long *)&mgp->stats)[i];
+ data[i++] = (unsigned int)mgp->tx.boundary;
+ data[i++] = (unsigned int)(mgp->mtrr >= 0);
+ data[i++] = (unsigned int)mgp->pdev->irq;
+ data[i++] = (unsigned int)mgp->msi_enabled;
data[i++] = (unsigned int)mgp->read_dma;
data[i++] = (unsigned int)mgp->write_dma;
data[i++] = (unsigned int)mgp->read_write_dma;
^ permalink raw reply
* [patch 0/4] myri10ge minor updates
From: brice @ 2006-07-03 22:02 UTC (permalink / raw)
To: netdev
Hi,
the following patches bring some minor updates for the myri10ge driver:
1) Drop unused pm_state
2) Drop ununsed nvidia chipset id
3) Use dev_info() when printing parameters after probe
4) Export more parameters to ethtool
Please apply.
thanks,
Brice
^ permalink raw reply
* Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats
From: Andrew Morton @ 2006-07-03 21:41 UTC (permalink / raw)
To: Shailabh Nagar
Cc: pj, Valdis.Kletnieks, jlan, balbir, csturtiv, linux-kernel, hadi,
netdev
In-Reply-To: <44A9881F.7030103@watson.ibm.com>
On Mon, 03 Jul 2006 17:11:59 -0400
Shailabh Nagar <nagar@watson.ibm.com> wrote:
> >>So the strawman is:
> >>Listener bind()s to genetlink using its real pid.
> >>Sends a separate "registration" message with cpumask to listen to.
> >>Kernel stores (real) pid and cpumask.
> >>During task exit, kernel goes through each registered listener (small
> >>list) and decides which
> >>one needs to get this exit data and calls a genetlink_unicast to each
> >>one that does need it.
> >>
> >>If number of listeners is small, the lookups should be swift enough. If
> >>it grows large, we
> >>can consider a fancier lookup (but there I go again, delving into
> >>implementation too early :-)
> >>
> >>
> >
> >We'll need a map.
> >
> >1024 CPUs, 1024 listeners, 1000 exits/sec/CPU and we're up to a million
> >operations per second per CPU. Meltdown.
> >
> >But it's a pretty simple map. A per-cpu array of pointers to the head of a
> >linked list. One lock for each CPU's list.
> >
> >
> Here's a patch that implements the above ideas.
>
> A listener register's interest by specifying a cpumask in the
> cpulist format (comma separated ranges of cpus). The listener's pid
> is entered into per-cpu lists for those cpus and exit events from those
> cpus go to the listeners using netlink unicasts.
>
> ...
>
> On systems with a large number of cpus, with even a modest rate of
> tasks exiting per cpu, the volume of taskstats data sent on thread exit
> can overflow a userspace listener's buffers.
>
> One approach to avoiding overflow is to allow listeners to get data for
> a limited and specific set of cpus. By scaling the number of listeners
> and/or the cpus they monitor, userspace can handle the statistical data
> overload more gracefully.
>
> In this patch, each listener registers to listen to a specific set of
> cpus by specifying a cpumask. The interest is recorded per-cpu. When
> a task exits on a cpu, its taskstats data is unicast to each listener
> interested in that cpu.
I think the approach is sane. The impementation needs work, as you say.
> +++ linux-2.6.17-mm3equiv/include/linux/taskstats_kern.h 2006-07-01 23:53:01.000000000 -0400
> @@ -19,20 +19,14 @@ enum {
> #ifdef CONFIG_TASKSTATS
> extern kmem_cache_t *taskstats_cache;
> extern struct mutex taskstats_exit_mutex;
> -
> -static inline int taskstats_has_listeners(void)
> -{
> - if (!genl_sock)
> - return 0;
> - return netlink_has_listeners(genl_sock, TASKSTATS_LISTEN_GROUP);
> -}
> -
> +DECLARE_PER_CPU(struct list_head, listener_list);
>
> static inline void taskstats_exit_alloc(struct taskstats **ptidstats)
> {
> *ptidstats = NULL;
> - if (taskstats_has_listeners())
> + if (!list_empty(&get_cpu_var(listener_list)))
> *ptidstats = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL);
> + put_cpu_var(listener_list);
> }
It's time to uninline this function..
> static inline void taskstats_exit_free(struct taskstats *tidstats)
> Index: linux-2.6.17-mm3equiv/kernel/taskstats.c
> ===================================================================
> --- linux-2.6.17-mm3equiv.orig/kernel/taskstats.c 2006-06-30 23:38:39.000000000 -0400
> +++ linux-2.6.17-mm3equiv/kernel/taskstats.c 2006-07-02 00:16:18.000000000 -0400
> @@ -19,6 +19,8 @@
> #include <linux/kernel.h>
> #include <linux/taskstats_kern.h>
> #include <linux/delayacct.h>
> +#include <linux/cpumask.h>
> +#include <linux/percpu.h>
> #include <net/genetlink.h>
> #include <asm/atomic.h>
>
> @@ -26,6 +28,9 @@ static DEFINE_PER_CPU(__u32, taskstats_s
> static int family_registered = 0;
> kmem_cache_t *taskstats_cache;
>
> +DEFINE_PER_CPU(struct list_head, listener_list);
> +static DEFINE_PER_CPU(struct rw_semaphore, listener_list_sem);
Which will permit listener_list to become static - it wasn't a good name
for a global anyway.
I suggest you implement a new
struct whatever {
struct rw_semaphore sem;
struct list_head list;
};
static DEFINE_PER_CPU(struct whatever, listener_aray);
> static int prepare_reply(struct genl_info *info, u8 cmd, struct sk_buff **skbp,
> void **replyp, size_t size)
> {
> @@ -77,6 +92,8 @@ static int prepare_reply(struct genl_inf
> static int send_reply(struct sk_buff *skb, pid_t pid, int event)
> {
> struct genlmsghdr *genlhdr = nlmsg_data((struct nlmsghdr *)skb->data);
> + struct rw_semaphore *sem;
> + struct list_head *p, *head;
> void *reply;
> int rc;
>
> @@ -88,9 +105,30 @@ static int send_reply(struct sk_buff *sk
> return rc;
> }
>
> - if (event == TASKSTATS_MSG_MULTICAST)
> - return genlmsg_multicast(skb, pid, TASKSTATS_LISTEN_GROUP);
> - return genlmsg_unicast(skb, pid);
> + if (event == TASKSTATS_MSG_UNICAST)
> + return genlmsg_unicast(skb, pid);
> +
> + /*
> + * Taskstats multicast is unicasts to listeners who have registered
> + * interest in this cpu
> + */
> + sem = &get_cpu_var(listener_list_sem);
> + head = &get_cpu_var(listener_list);
This has a double preempt_disable(), but the above will fix that.
> + down_read(sem);
> + list_for_each(p, head) {
> + int ret;
> + struct listener *s = list_entry(p, struct listener, list);
> + ret = genlmsg_unicast(skb, s->pid);
> + if (ret)
> + rc = ret;
> + }
> + up_read(sem);
> +
> + put_cpu_var(listener_list);
> + put_cpu_var(listener_list_sem);
> +
> + return rc;
> }
>
> static int fill_pid(pid_t pid, struct task_struct *pidtsk,
> @@ -201,8 +239,73 @@ ret:
> return;
> }
>
> +static int add_del_listener(pid_t pid, cpumask_t *maskp, int isadd)
> +{
> + struct listener *s;
> + unsigned int cpu, mycpu;
> + cpumask_t mask;
> + struct rw_semaphore *sem;
> + struct list_head *head, *p;
>
> -static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
> + memcpy(&mask, maskp, sizeof(cpumask_t));
> + if (cpus_empty(mask))
> + return -EINVAL;
> +
> + mycpu = get_cpu();
> + put_cpu();
This is effectively raw_smp_processor_id(). And after the put_cpu(),
`mycpu' is meaningless.
> + if (isadd == REGISTER) {
> + for_each_cpu_mask(cpu, mask) {
> + if (!cpu_possible(cpu))
> + continue;
> + if (cpu == mycpu)
> + preempt_disable();
> +
> + sem = &per_cpu(listener_list_sem, cpu);
> + head = &per_cpu(listener_list, cpu);
> +
> + s = kmalloc(sizeof(struct listener), GFP_KERNEL);
Cannot do GFP_KERNEL inside preempt_disable().
There's no easy solution to this problem. GFP_ATOMIC is not a good fix at
all. One approach would be to run lock_cpu_hotplug(), then allocate (with
GFP_KERNEL) all the memory which will be needed within the locked region,
then take the lock, then use that preallocated memory.
You should use kmalloc_node() here, to ensure that the memory on each CPU's
list resides with that CPU's local memory (not _this_ CPU's local memory).
> + if (!s)
> + return -ENOMEM;
> + s->pid = pid;
> + INIT_LIST_HEAD(&s->list);
> +
> + down_write(sem);
> + list_add(&s->list, head);
> + up_write(sem);
> +
> + if (cpu == mycpu)
> + preempt_enable();
Actually, I don't understand the tricks which are going on with the local CPU here.
What's it all for?
> + }
> + } else {
> + for_each_cpu_mask(cpu, mask) {
> + struct list_head *tmp;
> +
> + if (!cpu_possible(cpu))
> + continue;
I guess you could just do cpus_and(mask, cpus_possible_map) on entry.
> + if (cpu == mycpu)
> + preempt_disable();
> +
> + sem = &per_cpu(listener_list_sem, cpu);
> + head = &per_cpu(listener_list, cpu);
> +
> + down_write(sem);
> + list_for_each_safe(p, tmp, head) {
> + s = list_entry(p, struct listener, list);
> + if (s->pid == pid) {
> + list_del(&s->list);
kfree(s);
> + break;
> + }
> + }
> + up_write(sem);
> +
> + if (cpu == mycpu)
> + preempt_enable();
> + }
> + }
> + return 0;
> +}
> +
> +static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
> {
> int rc = 0;
> struct sk_buff *rep_skb;
> @@ -210,6 +313,21 @@ static int taskstats_send_stats(struct s
> void *reply;
> size_t size;
> struct nlattr *na;
> + cpumask_t mask;
> +
> + if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) {
> + na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
> + cpulist_parse((char *)nla_data(na), mask);
OK, so we're passing in an ASCII string. Fair enough, I think. Paul would
know better.
^ permalink raw reply
* Re: Two I/O memory regions in /proc/iomem for a NIC
From: Lennert Buytenhek @ 2006-07-03 21:30 UTC (permalink / raw)
To: John Que; +Cc: netdev
In-Reply-To: <ada605fb0607020837q5573299ke05864e130169bed@mail.gmail.com>
On Sun, Jul 02, 2006 at 06:37:18PM +0300, John Que wrote:
> Could a single call to
> pci_request_regions(pdev, driver_name)
> result in that we see 2 regions afterwards when running
> cat /proc/iomem?
Sure. Take a look at the output of 'lspci -v' some time. On my
machine, for example, I have:
05:11.0 Ethernet controller: Intel Corporation 82546GB Gigabit Ethernet Controller (rev 03)
Subsystem: Intel Corporation PRO/1000 MT Dual Port Network Connection
Flags: bus master, 66Mhz, medium devsel, latency 64, IRQ 10
Memory at cffc0000 (64-bit, non-prefetchable) [size=128K] <===
Memory at cff00000 (64-bit, non-prefetchable) [size=256K] <===
I/O ports at e400 [size=64]
Expansion ROM at cfec0000 [disabled] [size=256K]
Capabilities: <available only to root>
05:11.1 Ethernet controller: Intel Corporation 82546GB Gigabit Ethernet Controller (rev 03)
Subsystem: Intel Corporation PRO/1000 MT Dual Port Network Connection
Flags: bus master, 66Mhz, medium devsel, latency 64, IRQ 11
Memory at cffe0000 (64-bit, non-prefetchable) [size=128K] <===
Memory at cff80000 (64-bit, non-prefetchable) [size=256K] <===
I/O ports at e800 [size=64]
Expansion ROM at cff40000 [disabled] [size=256K]
Capabilities: <available only to root>
As you can see, each port of the dual-port card has two memory regions,
four a total of 4 memory regions.
cheers,
Lennert
^ 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