* Re: [RFC] sky2: don't request unused i/o region
From: Jeff Garzik @ 2008-02-14 4:40 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20080213190237.7cd49d60@extreme>
Stephen Hemminger wrote:
> The sky2 driver only uses the PCI memory region (0) not the
> available I/O region. Some users want to use lots of boards, and the
> I/O space gets exhausted.
>
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
>
> --- a/drivers/net/sky2.c 2008-02-13 18:58:21.000000000 -0800
> +++ b/drivers/net/sky2.c 2008-02-13 18:58:55.000000000 -0800
> @@ -4135,9 +4135,9 @@ static int __devinit sky2_probe(struct p
> goto err_out;
> }
>
> - err = pci_request_regions(pdev, DRV_NAME);
> + err = pci_request_region(pdev, 0, DRV_NAME);
> if (err) {
> - dev_err(&pdev->dev, "cannot obtain PCI resources\n");
> + dev_err(&pdev->dev, "cannot obtain PCI resource\n");
Your description of the problem does not match the fix.
PCI resources are already allocated to the device (or not) by this point
in the code. pci_request_region/regions is purely internal kernel
software resource reservation -- protecting drivers from themselves, and
arguably not really needed anymore on modern buses.
Thus, I cannot see how this patch can possibly "exhaust I/O space" --
the relevant PCI resources are allocated to the device, or not,
regardless of these function calls.
As long as there is no resource conflict, you can have 1 million boards
and still use pci_request_regions().
Jeff
^ permalink raw reply
* Re: [PATCH 2/2] add rcu_assign_index() if ever needed
From: Andrew Morton @ 2008-02-14 3:41 UTC (permalink / raw)
To: ego
Cc: Paul E. McKenney, linux-kernel, shemminger, davem, netdev,
dipankar, herbert
In-Reply-To: <20080214033209.GA7266@in.ibm.com>
On Thu, 14 Feb 2008 09:02:09 +0530 Gautham R Shenoy <ego@in.ibm.com> wrote:
> > /**
> > + * rcu_assign_index - assign (publicize) a index of a newly
> > + * initialized array elementg that will be dereferenced by RCU
> ^^^^^^^^
>
> I hope Andrew got that one while porting against the latest -mm :)
I don't actually read the comments - I just like to make sure they're
there ;)
^ permalink raw reply
* Re: [PATCH 2/2] add rcu_assign_index() if ever needed
From: Gautham R Shenoy @ 2008-02-14 3:32 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, shemminger, davem, netdev, dipankar, herbert, akpm
In-Reply-To: <20080213220515.GA10642@linux.vnet.ibm.com>
On Wed, Feb 13, 2008 at 02:05:15PM -0800, Paul E. McKenney wrote:
> Hello again!
>
> This is a speculative patch that as far as I can tell is not yet required.
> If anyone applies RCU to a data structure allocated out of an array, using
> array indexes in place of pointers to link the array elements together,
> then the rcu_assign_index() function in this patch will be needed to
> assign a given element's array index to the RCU-traversed index. The
> implementation is exactly that of the old rcu_assign_pointer(), so is
> extremely well tested.
>
> The existing rcu_assign_pointer() will emit a compiler warning in cases
> where rcu_assign_index() is required.
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
> rcupdate.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff -urpNa -X dontdiff linux-2.6.24-rap/include/linux/rcupdate.h linux-2.6.24-rai/include/linux/rcupdate.h
> --- linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> +++ linux-2.6.24-rai/include/linux/rcupdate.h 2008-02-13 10:55:40.000000000 -0800
> @@ -286,6 +286,24 @@ extern struct lockdep_map rcu_lock_map;
> })
>
> /**
> + * rcu_assign_index - assign (publicize) a index of a newly
> + * initialized array elementg that will be dereferenced by RCU
^^^^^^^^
I hope Andrew got that one while porting against the latest -mm :)
Looks good otherwise.
> + * read-side critical sections. Returns the value assigned.
> + *
> + * Inserts memory barriers on architectures that require them
> + * (pretty much all of them other than x86), and also prevents
> + * the compiler from reordering the code that initializes the
> + * structure after the index assignment. More importantly, this
> + * call documents which indexes will be dereferenced by RCU read-side
> + * code.
> + */
> +
> +#define rcu_assign_index(p, v) ({ \
> + smp_wmb(); \
> + (p) = (v); \
> + })
> +
> +/**
> * synchronize_sched - block until all CPUs have exited any non-preemptive
> * kernel code sequences.
> *
--
Thanks and Regards
gautham
^ permalink raw reply
* [PATCH 2.6.25] igb: fix legacy mode irq issue
From: Andy Gospodarek @ 2008-02-14 3:19 UTC (permalink / raw)
To: netdev; +Cc: Auke Kok, David S. Miller, Jeff Garzik
I booted an igb kernel with the option pci=nomsi and instantly noticed
that interrupts no longer worked on my igb device. I took a look at the
interrupt initialization and quickly discovered a comment stating:
"DO NOT USE EIAME or IAME in legacy mode"
It seemed a bit odd that bits to enable IAM were being set in legacy
interrupt mode, so I dropped out the following parts and interrupts
began working fine again.
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---
igb_main.c | 3 ---
1 files changed, 3 deletions(-)
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index f3c144d..be5da09 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -472,9 +471,6 @@ static int igb_request_irq(struct igb_adapter *adapter)
goto request_done;
}
- /* enable IAM, auto-mask */
- wr32(E1000_IAM, IMS_ENABLE_MASK);
-
request_done:
return err;
}
^ permalink raw reply related
* [RFC] sky2: don't request unused i/o region
From: Stephen Hemminger @ 2008-02-14 3:02 UTC (permalink / raw)
To: Jeff Garzik; +Cc: netdev
The sky2 driver only uses the PCI memory region (0) not the
available I/O region. Some users want to use lots of boards, and the
I/O space gets exhausted.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/drivers/net/sky2.c 2008-02-13 18:58:21.000000000 -0800
+++ b/drivers/net/sky2.c 2008-02-13 18:58:55.000000000 -0800
@@ -4135,9 +4135,9 @@ static int __devinit sky2_probe(struct p
goto err_out;
}
- err = pci_request_regions(pdev, DRV_NAME);
+ err = pci_request_region(pdev, 0, DRV_NAME);
if (err) {
- dev_err(&pdev->dev, "cannot obtain PCI resources\n");
+ dev_err(&pdev->dev, "cannot obtain PCI resource\n");
goto err_out_disable;
}
^ permalink raw reply
* Dealing with limited resources and DMA Engine copies
From: Olof Johansson @ 2008-02-14 2:38 UTC (permalink / raw)
To: shannon.nelson, dan.j.williams; +Cc: netdev
Hi,
My DMA Engine has a limited resource: It's got a descriptor ring, so
it's not always possible to add a new descriptor to it (i.e. it might be
full). While allocating a huge ring will help, eventually I'm sure I
will hit a case where it'll overflow.
I thought this was going to be taken care of automatically by the fact
that you return your max(?) number of descriptors in the channel
allocation function, but it looks like that value is discarded in
dma_client_chan_alloc().
So, I just got a couple of spurious:
dma_cookie < 0
dma_cookie < 0
...on the console and the connection terminated. Looks like that came
from tcp_recvmsg(). Ouch.
How about falling back to the cpu-based copy in case of failure? Or would
you prefer that I sleep locally in my driver and wait on a descriptor
slot to open up?
-Olof
^ permalink raw reply
* Re: [2.6 patch] unexport inet_listen_wlock
From: David Miller @ 2008-02-14 1:40 UTC (permalink / raw)
To: bunk; +Cc: netdev
In-Reply-To: <20080213212948.GL3383@cs181133002.pp.htv.fi>
From: Adrian Bunk <bunk@kernel.org>
Date: Wed, 13 Feb 2008 23:29:48 +0200
> This patch removes the no linger used EXPORT_SYMBOL(inet_listen_wlock).
>
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
Applied.
^ permalink raw reply
* Re: [2.6 patch] unexport __inet_hash_connect
From: David Miller @ 2008-02-14 1:39 UTC (permalink / raw)
To: bunk; +Cc: xemul, linux-kernel, netdev
In-Reply-To: <20080213212946.GK3383@cs181133002.pp.htv.fi>
From: Adrian Bunk <bunk@kernel.org>
Date: Wed, 13 Feb 2008 23:29:46 +0200
> This patch removes the unused EXPORT_SYMBOL_GPL(__inet_hash_connect).
>
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
Applied.
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-14 1:37 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080214013427.GT12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 17:34:27 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 04:53:56PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 16:42:53 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Feb 13, 2008 at 04:27:00PM -0800, Stephen Hemminger wrote:
>
> [ . . . ]
>
> > > > That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
> > > >
> > > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > > {
> > > > smp_wmb();
> > > > node->parent = (unsigned long)ptr | NODE_TYPE(node);
> > > > }
> > >
> > > Or, alternatively, the rcu_assign_index() patch sent earlier to avoid
> > > the bare memory barrier?
> > >
> > > Thanx, Paul
> >
> > I am fine with rcu_assign_index(), and add a comment in node_set_parent.
>
> OK, how about the following?
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
> fib_trie.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 17:31:16.000000000 -0800
> @@ -96,6 +96,14 @@ typedef unsigned int t_key;
> #define IS_TNODE(n) (!(n->parent & T_LEAF))
> #define IS_LEAF(n) (n->parent & T_LEAF)
>
> +/*
> + * The "parent" fields in struct node and struct leaf are really pointers,
> + * but with the possibility that the T_LEAF bit is set. Therefore, both
> + * the C compiler and RCU see them as integers rather than pointers.
> + * This in turn means that rcu_assign_index() must be used to assign
> + * values to these fields, rather than the usual rcu_assign_pointer().
> + */
> +
> struct node {
> unsigned long parent;
> t_key key;
> @@ -179,8 +187,7 @@ static inline struct tnode *node_parent_
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> - rcu_assign_pointer(node->parent,
> - (unsigned long)ptr | NODE_TYPE(node));
> + rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> }
>
> static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
Yes, thats great.
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-14 1:34 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213165356.11d02092@extreme>
On Wed, Feb 13, 2008 at 04:53:56PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 16:42:53 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Feb 13, 2008 at 04:27:00PM -0800, Stephen Hemminger wrote:
[ . . . ]
> > > That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > smp_wmb();
> > > node->parent = (unsigned long)ptr | NODE_TYPE(node);
> > > }
> >
> > Or, alternatively, the rcu_assign_index() patch sent earlier to avoid
> > the bare memory barrier?
> >
> > Thanx, Paul
>
> I am fine with rcu_assign_index(), and add a comment in node_set_parent.
OK, how about the following?
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
fib_trie.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
--- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
+++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 17:31:16.000000000 -0800
@@ -96,6 +96,14 @@ typedef unsigned int t_key;
#define IS_TNODE(n) (!(n->parent & T_LEAF))
#define IS_LEAF(n) (n->parent & T_LEAF)
+/*
+ * The "parent" fields in struct node and struct leaf are really pointers,
+ * but with the possibility that the T_LEAF bit is set. Therefore, both
+ * the C compiler and RCU see them as integers rather than pointers.
+ * This in turn means that rcu_assign_index() must be used to assign
+ * values to these fields, rather than the usual rcu_assign_pointer().
+ */
+
struct node {
unsigned long parent;
t_key key;
@@ -179,8 +187,7 @@ static inline struct tnode *node_parent_
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
- rcu_assign_pointer(node->parent,
- (unsigned long)ptr | NODE_TYPE(node));
+ rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
}
static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
^ permalink raw reply
* [PATCH] Fix tcp_v4_send_synack() comment
From: Kris Katterjohn @ 2008-02-14 1:03 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 188 bytes --]
Hey everyone,
I've attached a patch that fixes the comment above tcp_v4_send_synack()
in ipv4/tcp_ipv4.c.
Signed-off-by: Kris Katterjohn <katterjohn@gmail.com>
Thanks,
Kris Katterjohn
[-- Attachment #2: synack.patch --]
[-- Type: text/x-patch, Size: 365 bytes --]
--- net/ipv4/tcp_ipv4.c 2008-02-10 23:51:11.000000000 -0600
+++ net/ipv4/tcp_ipv4.c 2008-02-13 18:31:44.000000000 -0600
@@ -735,7 +735,7 @@ static void tcp_v4_reqsk_send_ack(struct
}
/*
- * Send a SYN-ACK after having received an ACK.
+ * Send a SYN-ACK after having received a SYN.
* This still operates on a request_sock only, not on a big
* socket.
*/
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-14 0:53 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080214004253.GR12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 16:42:53 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 04:27:00PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 16:14:04 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
>
> [ . . . ]
>
> > > > Maybe cast both sides to void * in this case:
> > > >
> > > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > > {
> > > > rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > > > }
> > >
> > > That gets me the following:
> > >
> > > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > > net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
> > >
> > > However, as with much in computing, an extra level of indirection fixes
> > > things. Your call as to whether or not the cure is preferable to the
> > > disease. ;-)
> > >
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> > >
> > > fib_trie.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> > > --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> > > +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
> > > @@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > - rcu_assign_pointer(node->parent,
> > > - (unsigned long)ptr | NODE_TYPE(node));
> > > + rcu_assign_pointer((*(void **)&node->parent),
> > > + (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > > }
> >
> > That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > smp_wmb();
> > node->parent = (unsigned long)ptr | NODE_TYPE(node);
> > }
>
> Or, alternatively, the rcu_assign_index() patch sent earlier to avoid
> the bare memory barrier?
>
> Thanx, Paul
I am fine with rcu_assign_index(), and add a comment in node_set_parent.
^ permalink raw reply
* Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
From: Jann Traschewski @ 2008-02-14 0:49 UTC (permalink / raw)
To: 'Jarek Poplawski', 'David Miller'
Cc: 'Bernard Pidoux F6BVP', 'Ralf Baechle', netdev
In-Reply-To: <20080213115607.GB2867@ff.dom.local>
Applied and stable with Kernel 2.6.24.2 since 12 hours.
Regards,
Jann
> -----Ursprüngliche Nachricht-----
> Von: Jarek Poplawski [mailto:jarkao2@gmail.com]
> Gesendet: Mittwoch, 13. Februar 2008 12:56
> An: David Miller
> Cc: Jann Traschewski; Bernard Pidoux F6BVP; Ralf Baechle;
> netdev@vger.kernel.org
> Betreff: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
>
> Hi,
>
> Here is an "official" version of "testing patch #2" from this thread.
> The only difference: ax25->vs is changed only after checking
> skb is not NULL (plus a comment). IMHO it could be applied.
>
> Thanks,
> Jarek P.
>
> ---------------->
>
> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
>
> According to some OOPS reports ax25_kick tries to clone NULL
> skbs sometimes. It looks like a race with
> ax25_clear_queues(). Probably there is no need to add more
> than a simple check for this yet.
> Another report suggested there are probably also cases where ax25
> ->paclen == 0 can happen in ax25_output(); this wasn't confirmed
> during testing but let's leave this debugging check for some time.
>
>
> Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>
> ---
>
> diff -Nurp 2.6.24-mm1-/net/ax25/ax25_out.c
> 2.6.24-mm1+/net/ax25/ax25_out.c
> --- 2.6.24-mm1-/net/ax25/ax25_out.c 2008-01-24
> 22:58:37.000000000 +0000
> +++ 2.6.24-mm1+/net/ax25/ax25_out.c 2008-02-13
> 10:43:50.000000000 +0000
> @@ -117,6 +117,12 @@ void ax25_output(ax25_cb *ax25, int pacl
> unsigned char *p;
> int frontlen, len, fragno, ka9qfrag, first = 1;
>
> + if (paclen < 16) {
> + WARN_ON_ONCE(1);
> + kfree_skb(skb);
> + return;
> + }
> +
> if ((skb->len - 1) > paclen) {
> if (*skb->data == AX25_P_TEXT) {
> skb_pull(skb, 1); /* skip PID */
> @@ -251,8 +257,6 @@ void ax25_kick(ax25_cb *ax25)
> if (start == end)
> return;
>
> - ax25->vs = start;
> -
> /*
> * Transmit data until either we're out of data to send or
> * the window is full. Send a poll on the final I frame
> if @@ -261,8 +265,13 @@ void ax25_kick(ax25_cb *ax25)
>
> /*
> * Dequeue the frame and copy it.
> + * Check for race with ax25_clear_queues().
> */
> skb = skb_dequeue(&ax25->write_queue);
> + if (!skb)
> + return;
> +
> + ax25->vs = start;
>
> do {
> if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {
^ permalink raw reply
* [patch] Fix station address detection in smc
From: Chuck Ebbert @ 2008-02-14 0:47 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: linux-pcmcia, Netdev, Jeff Garzik
Megahertz EM1144 PCMCIA ethernet adapter needs special handling
because it has two VERS_1 tuples and the station address is in
the second one. Conversion to generic handling of these fields
broke it. Reverting that fixes the device.
https://bugzilla.redhat.com/show_bug.cgi?id=233255
Thanks go to Jon Stanley for not giving up on this one until the
problem was found.
Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
---
This was broken for over a year before the problem was found...
--- b/drivers/net/pcmcia/smc91c92_cs.c Tue Dec 05 06:09:21 2006 +0500
+++ a/drivers/net/pcmcia/smc91c92_cs.c Tue Dec 05 06:09:20 2006 +0500
@@ -560,8 +560,16 @@
/* Read the station address from the CIS. It is stored as the last
(fourth) string in the Version 1 Version/ID tuple. */
+ tuple->DesiredTuple = CISTPL_VERS_1;
+ if (first_tuple(link, tuple, parse) != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
+ }
+ /* Ugh -- the EM1144 card has two VERS_1 tuples!?! */
+ if (next_tuple(link, tuple, parse) != CS_SUCCESS)
+ first_tuple(link, tuple, parse);
+ if (parse->version_1.ns > 3) {
+ station_addr = parse->version_1.str + parse->version_1.ofs[3];
- if (link->prod_id[3]) {
- station_addr = link->prod_id[3];
if (cvt_ascii_address(dev, station_addr) == 0) {
rc = 0;
goto free_cfg_mem;
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-14 0:42 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213162700.0a32000d@extreme>
On Wed, Feb 13, 2008 at 04:27:00PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 16:14:04 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
[ . . . ]
> > > Maybe cast both sides to void * in this case:
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > > }
> >
> > That gets me the following:
> >
> > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
> >
> > However, as with much in computing, an extra level of indirection fixes
> > things. Your call as to whether or not the cure is preferable to the
> > disease. ;-)
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >
> > fib_trie.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> > --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> > +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
> > @@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > - rcu_assign_pointer(node->parent,
> > - (unsigned long)ptr | NODE_TYPE(node));
> > + rcu_assign_pointer((*(void **)&node->parent),
> > + (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > }
>
> That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> smp_wmb();
> node->parent = (unsigned long)ptr | NODE_TYPE(node);
> }
Or, alternatively, the rcu_assign_index() patch sent earlier to avoid
the bare memory barrier?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-14 0:27 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080214001404.GQ12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 16:14:04 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 15:37:44 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> > > > On Wed, 13 Feb 2008 14:41:34 -0800
> > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > > > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > > > >
> > > > > > > Hello!
> > > > > > >
> > > > > > > This is an updated version of the patch posted last November:
> > > > > > >
> > > > > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > > > > >
> > > > > > > This new version permits arguments with side effects, for example:
> > > > > > >
> > > > > > > rcu_assign_pointer(global_p, p++);
> > > > > > >
> > > > > > > and also verifies that the arguments are pointers, while still avoiding
> > > > > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > > > > permitted for pointers (not array indexes), and so this version emits a
> > > > > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > > > > >
> > > > > > > Given the rather surprising history of subtlely broken implementations of
> > > > > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > > > > test cases and verified that memory barriers and compiler warnings were
> > > > > > > emitted when required. I guess it is the simple things that get you...
> > > > > > >
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >
> > > > > > > rcupdate.h | 16 ++++++++++++----
> > > > > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > > > > * structure after the pointer assignment. More importantly, this
> > > > > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > > > > * code.
> > > > > > > + *
> > > > > > > + * Throws a compiler warning for non-pointer arguments.
> > > > > > > + *
> > > > > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > > > > */
> > > > > > >
> > > > > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > > > > - smp_wmb(); \
> > > > > > > - (p) = (v); \
> > > > > > > - })
> > > > > > > +#define rcu_assign_pointer(p, v) \
> > > > > > > + ({ \
> > > > > > > + typeof(*p) *_________p1 = (v); \
> > > > > > > + \
> > > > > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > > > > + smp_wmb(); \
> > > > > > > + (p) = _________p1; \
> > > > > > > + })
> > > > > > >
> > > > > > > /**
> > > > > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > > > > >
> > > > > > Will this still work if p is unsigned long?
> > > > >
> > > > > Hello, Steve,
> > > > >
> > > > > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > > > > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > > > > that he is aware if you do use it.
> > > >
> > > > Make sure fib_trie still works and doesn't get warnings.
> > >
> > > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > > doesn't cut it. Please accept my apologies for my confusion!!!
> > >
> > > Once fib_trie is configured, I do indeed get:
> > >
> > > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > > net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
> > >
> > > So, given that node->parent is an unsigned long, I changed node_set_parent()
> > > to the following:
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> > > }
> > >
> > > This removes the warnings. I am a little ambivalent about this, as
> > > this is really a pointer in disguise rather than an array index, but
> > > patch below. I suppose that another option would be to make node->parent
> > > be a void* and provide appropriate accessor functions/macros.
> > >
> > > Thoughts?
> > >
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> > Maybe cast both sides to void * in this case:
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > }
>
> That gets me the following:
>
> net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
>
> However, as with much in computing, an extra level of indirection fixes
> things. Your call as to whether or not the cure is preferable to the
> disease. ;-)
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
> fib_trie.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
> @@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> - rcu_assign_pointer(node->parent,
> - (unsigned long)ptr | NODE_TYPE(node));
> + rcu_assign_pointer((*(void **)&node->parent),
> + (void *)((unsigned long)ptr | NODE_TYPE(node)));
> }
That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
smp_wmb();
node->parent = (unsigned long)ptr | NODE_TYPE(node);
}
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-14 0:14 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213155158.1b621359@extreme>
On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 15:37:44 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> > > On Wed, 13 Feb 2008 14:41:34 -0800
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > > >
> > > > > > Hello!
> > > > > >
> > > > > > This is an updated version of the patch posted last November:
> > > > > >
> > > > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > > > >
> > > > > > This new version permits arguments with side effects, for example:
> > > > > >
> > > > > > rcu_assign_pointer(global_p, p++);
> > > > > >
> > > > > > and also verifies that the arguments are pointers, while still avoiding
> > > > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > > > permitted for pointers (not array indexes), and so this version emits a
> > > > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > > > >
> > > > > > Given the rather surprising history of subtlely broken implementations of
> > > > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > > > test cases and verified that memory barriers and compiler warnings were
> > > > > > emitted when required. I guess it is the simple things that get you...
> > > > > >
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >
> > > > > > rcupdate.h | 16 ++++++++++++----
> > > > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > > > * structure after the pointer assignment. More importantly, this
> > > > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > > > * code.
> > > > > > + *
> > > > > > + * Throws a compiler warning for non-pointer arguments.
> > > > > > + *
> > > > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > > > */
> > > > > >
> > > > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > > > - smp_wmb(); \
> > > > > > - (p) = (v); \
> > > > > > - })
> > > > > > +#define rcu_assign_pointer(p, v) \
> > > > > > + ({ \
> > > > > > + typeof(*p) *_________p1 = (v); \
> > > > > > + \
> > > > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > > > + smp_wmb(); \
> > > > > > + (p) = _________p1; \
> > > > > > + })
> > > > > >
> > > > > > /**
> > > > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > > > >
> > > > > Will this still work if p is unsigned long?
> > > >
> > > > Hello, Steve,
> > > >
> > > > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > > > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > > > that he is aware if you do use it.
> > >
> > > Make sure fib_trie still works and doesn't get warnings.
> >
> > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > doesn't cut it. Please accept my apologies for my confusion!!!
> >
> > Once fib_trie is configured, I do indeed get:
> >
> > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
> >
> > So, given that node->parent is an unsigned long, I changed node_set_parent()
> > to the following:
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> > }
> >
> > This removes the warnings. I am a little ambivalent about this, as
> > this is really a pointer in disguise rather than an array index, but
> > patch below. I suppose that another option would be to make node->parent
> > be a void* and provide appropriate accessor functions/macros.
> >
> > Thoughts?
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> Maybe cast both sides to void * in this case:
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> }
That gets me the following:
net/ipv4/fib_trie.c: In function ‘node_set_parent’:
net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
However, as with much in computing, an extra level of indirection fixes
things. Your call as to whether or not the cure is preferable to the
disease. ;-)
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
fib_trie.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
--- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
+++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
@@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
- rcu_assign_pointer(node->parent,
- (unsigned long)ptr | NODE_TYPE(node));
+ rcu_assign_pointer((*(void **)&node->parent),
+ (void *)((unsigned long)ptr | NODE_TYPE(node)));
}
static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Andrew Morton @ 2008-02-14 0:09 UTC (permalink / raw)
To: David Miller
Cc: paulmck, "stephen.hemminger, linux-kernel, shemminger, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213.155738.71849437.davem@davemloft.net>
On Wed, 13 Feb 2008 15:57:38 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Wed, 13 Feb 2008 15:52:45 -0800
>
> > On Wed, 13 Feb 2008 15:37:44 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > > doesn't cut it.
> >
> > This is not good. The sole purpose of allmodconfig and allyesconfig is for
> > compilation and linkage coverage testing. Hence we should aim to get as
> > much code as possible included in those cases.
>
> Well, in this case there is a choice, either you use one routing
> lookup datastructure or the other. It's not purposefully being hidden
> from the everything builds :-)
oic. yes, that is a bit of a problem. Oh well.
`make randconfig' seems to be able to enable CONFIG_IP_FIB_TRIE about one
time in eight ;)
^ permalink raw reply
* Network namespace and tc?
From: Stephen Hemminger @ 2008-02-13 23:59 UTC (permalink / raw)
To: netdev
It looks like tc filter won't work on alternate namespaces:
/* Add/change/delete/get a filter node */
static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
{
...
if (net != &init_net)
return -EINVAL;
Haven't played with namespace virtualization yet, but what else is
not supported? Where is this documented?
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: David Miller @ 2008-02-13 23:57 UTC (permalink / raw)
To: akpm
Cc: paulmck, "stephen.hemminger, linux-kernel, shemminger, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213155245.455944f8.akpm@linux-foundation.org>
From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 13 Feb 2008 15:52:45 -0800
> On Wed, 13 Feb 2008 15:37:44 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > doesn't cut it.
>
> This is not good. The sole purpose of allmodconfig and allyesconfig is for
> compilation and linkage coverage testing. Hence we should aim to get as
> much code as possible included in those cases.
Well, in this case there is a choice, either you use one routing
lookup datastructure or the other. It's not purposefully being hidden
from the everything builds :-)
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-13 23:55 UTC (permalink / raw)
To: Andrew Morton
Cc: paulmck, "stephen.hemminger, linux-kernel, davem, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213155245.455944f8.akpm@linux-foundation.org>
On Wed, 13 Feb 2008 15:52:45 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 13 Feb 2008 15:37:44 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > doesn't cut it.
>
> This is not good. The sole purpose of allmodconfig and allyesconfig is for
> compilation and linkage coverage testing. Hence we should aim to get as
> much code as possible included in those cases.
>
The current model is compile time choice. It is on my long term list
to make this a runtime option.
--
Stephen Hemminger <stephen.hemminger@vyatta.com>
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Andrew Morton @ 2008-02-13 23:52 UTC (permalink / raw)
To: paulmck
Cc: "stephen.hemminger, linux-kernel, shemminger, davem, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213233744.GO12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 15:37:44 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> doesn't cut it.
This is not good. The sole purpose of allmodconfig and allyesconfig is for
compilation and linkage coverage testing. Hence we should aim to get as
much code as possible included in those cases.
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-13 23:51 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213233744.GO12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 15:37:44 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 14:41:34 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > > Hello!
> > > > >
> > > > > This is an updated version of the patch posted last November:
> > > > >
> > > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > > >
> > > > > This new version permits arguments with side effects, for example:
> > > > >
> > > > > rcu_assign_pointer(global_p, p++);
> > > > >
> > > > > and also verifies that the arguments are pointers, while still avoiding
> > > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > > permitted for pointers (not array indexes), and so this version emits a
> > > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > > >
> > > > > Given the rather surprising history of subtlely broken implementations of
> > > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > > test cases and verified that memory barriers and compiler warnings were
> > > > > emitted when required. I guess it is the simple things that get you...
> > > > >
> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > ---
> > > > >
> > > > > rcupdate.h | 16 ++++++++++++----
> > > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > > * structure after the pointer assignment. More importantly, this
> > > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > > * code.
> > > > > + *
> > > > > + * Throws a compiler warning for non-pointer arguments.
> > > > > + *
> > > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > > */
> > > > >
> > > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > > - smp_wmb(); \
> > > > > - (p) = (v); \
> > > > > - })
> > > > > +#define rcu_assign_pointer(p, v) \
> > > > > + ({ \
> > > > > + typeof(*p) *_________p1 = (v); \
> > > > > + \
> > > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > > + smp_wmb(); \
> > > > > + (p) = _________p1; \
> > > > > + })
> > > > >
> > > > > /**
> > > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > > >
> > > > Will this still work if p is unsigned long?
> > >
> > > Hello, Steve,
> > >
> > > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > > that he is aware if you do use it.
> >
> > Make sure fib_trie still works and doesn't get warnings.
>
> Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> doesn't cut it. Please accept my apologies for my confusion!!!
>
> Once fib_trie is configured, I do indeed get:
>
> net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
>
> So, given that node->parent is an unsigned long, I changed node_set_parent()
> to the following:
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> }
>
> This removes the warnings. I am a little ambivalent about this, as
> this is really a pointer in disguise rather than an array index, but
> patch below. I suppose that another option would be to make node->parent
> be a void* and provide appropriate accessor functions/macros.
>
> Thoughts?
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Maybe cast both sides to void * in this case:
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
}
--
Stephen Hemminger <stephen.hemminger@vyatta.com>
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-13 23:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: linux-kernel, shemminger, davem, netdev, dipankar, ego, herbert,
akpm
In-Reply-To: <20080213144233.05e860cb@extreme>
On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 14:41:34 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > Hello!
> > > >
> > > > This is an updated version of the patch posted last November:
> > > >
> > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > >
> > > > This new version permits arguments with side effects, for example:
> > > >
> > > > rcu_assign_pointer(global_p, p++);
> > > >
> > > > and also verifies that the arguments are pointers, while still avoiding
> > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > permitted for pointers (not array indexes), and so this version emits a
> > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > >
> > > > Given the rather surprising history of subtlely broken implementations of
> > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > test cases and verified that memory barriers and compiler warnings were
> > > > emitted when required. I guess it is the simple things that get you...
> > > >
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > ---
> > > >
> > > > rcupdate.h | 16 ++++++++++++----
> > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > * structure after the pointer assignment. More importantly, this
> > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > * code.
> > > > + *
> > > > + * Throws a compiler warning for non-pointer arguments.
> > > > + *
> > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > */
> > > >
> > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > - smp_wmb(); \
> > > > - (p) = (v); \
> > > > - })
> > > > +#define rcu_assign_pointer(p, v) \
> > > > + ({ \
> > > > + typeof(*p) *_________p1 = (v); \
> > > > + \
> > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > + smp_wmb(); \
> > > > + (p) = _________p1; \
> > > > + })
> > > >
> > > > /**
> > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > >
> > > Will this still work if p is unsigned long?
> >
> > Hello, Steve,
> >
> > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > that he is aware if you do use it.
>
> Make sure fib_trie still works and doesn't get warnings.
Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
doesn't cut it. Please accept my apologies for my confusion!!!
Once fib_trie is configured, I do indeed get:
net/ipv4/fib_trie.c: In function ‘node_set_parent’:
net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
So, given that node->parent is an unsigned long, I changed node_set_parent()
to the following:
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
}
This removes the warnings. I am a little ambivalent about this, as
this is really a pointer in disguise rather than an array index, but
patch below. I suppose that another option would be to make node->parent
be a void* and provide appropriate accessor functions/macros.
Thoughts?
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
fib_trie.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
--- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
+++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 15:22:02.000000000 -0800
@@ -179,8 +179,7 @@ static inline struct tnode *node_parent_
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
- rcu_assign_pointer(node->parent,
- (unsigned long)ptr | NODE_TYPE(node));
+ rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
}
static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
^ permalink raw reply
* Re: [PATCH] fib_trie: rcu_assign_pointer warning fix
From: Paul E. McKenney @ 2008-02-13 22:55 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, shemminger, netdev, linux-kernel
In-Reply-To: <20080212194630.GB2803@ami.dom.local>
On Tue, Feb 12, 2008 at 08:46:30PM +0100, Jarek Poplawski wrote:
> On Tue, Feb 12, 2008 at 08:32:18PM +0100, Jarek Poplawski wrote:
> ...
> > It seems the above version of this macro uses the barrier for 0, but
> > if I miss something, or for these other: documenting reasons,
>
> ...or __builtin_constants could be used for indexing (?!),
Yep. For example:
elem[0].next = 1;
rcu_assign_index(global_index, 0);
Thanx, Paul
> > then of
> > course you are right.
>
> Jarek P.
^ 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