| Message ID | 20090608131138.10052.5408.stgit@localhost |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Jesper Dangaard Brouer wrote: > This module uses rcu_call() thus it should use rcu_barrier() > on module unload. > > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk> Thanks Jesper for pointing at this issue! Acked-By: Oliver Hartkopp <oliver@hartkopp.net> Btw. i do agree with theses patches to be a bug fix that should go into 2.6.30-rc8 as well as into the stable series. Best regards, Oliver > --- > > net/can/af_can.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/net/can/af_can.c b/net/can/af_can.c > index 10f0528..e733725 100644 > --- a/net/can/af_can.c > +++ b/net/can/af_can.c > @@ -903,6 +903,8 @@ static __exit void can_exit(void) > } > spin_unlock(&can_rcvlists_lock); > > + rcu_barrier(); /* Wait for completion of call_rcu()'s */ > + > kmem_cache_destroy(rcv_cache); > } > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 08, 2009 at 03:11:38PM +0200, Jesper Dangaard Brouer wrote: > This module uses rcu_call() thus it should use rcu_barrier() > on module unload. This does appear to make things better!!! However, I don't understand why it is safe to do the following in can_exit(): hlist_for_each_entry_safe(d, n, next, &can_rx_dev_list, list) { hlist_del(&d->list); kfree(d); } Given that this list is scanned by RCU readers, shouldn't this kfree() be something like "call_rcu(&d->rcu, can_rx_delete_device);"? Also, what frees up the "struct receiver" structures? Thanx, Paul > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk> > --- > > net/can/af_can.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/net/can/af_can.c b/net/can/af_can.c > index 10f0528..e733725 100644 > --- a/net/can/af_can.c > +++ b/net/can/af_can.c > @@ -903,6 +903,8 @@ static __exit void can_exit(void) > } > spin_unlock(&can_rcvlists_lock); > > + rcu_barrier(); /* Wait for completion of call_rcu()'s */ > + > kmem_cache_destroy(rcv_cache); > } > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Paul E. McKenney wrote: > On Mon, Jun 08, 2009 at 03:11:38PM +0200, Jesper Dangaard Brouer wrote: >> This module uses rcu_call() thus it should use rcu_barrier() >> on module unload. > > This does appear to make things better!!! > > However, I don't understand why it is safe to do the following in > can_exit(): > > hlist_for_each_entry_safe(d, n, next, &can_rx_dev_list, list) { > hlist_del(&d->list); > kfree(d); > } > > Given that this list is scanned by RCU readers, shouldn't this kfree() > be something like "call_rcu(&d->rcu, can_rx_delete_device);"? > > Also, what frees up the "struct receiver" structures? Hi Paul, af_can.c only provides an infrastructure for PF_CAN modules like can-raw.ko, can-bcm.ko or can-isotp.ko. Please take a look into can_notifier() in net/can/af_can.c and raw_notifier() in net/can/raw.c: The receivers are removed when the appropriate socket is closed that created the belonging receivers. And you can not remove can.ko (af_can.c) when another PF_CAN protocol like can-raw.ko is using it. So when a netdev notifier removes the interface both the PF_CAN protocol (e.g. can-raw.ko) and the PF_CAN core (af_can.c) cleans up all receivers and finally removes the per-interface structure dev_rcv_lists (e.g. for can0). In can_exit() all the dev_rcv_lists for ARPHRD_CAN interfaces are removed that had been created by NETDEV_REGISTER notifier and are unused by any of the PF_CAN protocols and therefore without any receivers attached to them. The list is protected by spin_lock(&can_rcvlists_lock) - which is probably not even needed in this particular case - and there is no PF_CAN protocol registered at this time. So it's really save to remove the empty dev_rcv_lists structs here that do not link to any receivers. Puh - much text. But i hope it clarifies it. Thinking about the rcu stuff again, rcu_barrier() still makes sense when you are unloading the module chain of can-raw.ko and can.ko very fast. Regards, Oliver >> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk> >> --- >> >> net/can/af_can.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/net/can/af_can.c b/net/can/af_can.c >> index 10f0528..e733725 100644 >> --- a/net/can/af_can.c >> +++ b/net/can/af_can.c >> @@ -903,6 +903,8 @@ static __exit void can_exit(void) >> } >> spin_unlock(&can_rcvlists_lock); >> >> + rcu_barrier(); /* Wait for completion of call_rcu()'s */ >> + >> kmem_cache_destroy(rcv_cache); >> } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Hartkopp <oliver@hartkopp.net> Date: Mon, 08 Jun 2009 15:24:58 +0200 > Acked-By: Oliver Hartkopp <oliver@hartkopp.net> Please don't capitalize the "By" in "Acked-By". Otherwise patchwork doesn't pick them up automatically. It's just like "Signed-off-by", only the first word is capitalized. Thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Oliver Hartkopp <oliver@hartkopp.net> > Date: Mon, 08 Jun 2009 15:24:58 +0200 > >> Acked-By: Oliver Hartkopp <oliver@hartkopp.net> > > Please don't capitalize the "By" in "Acked-By". Sorry. > Otherwise > patchwork doesn't pick them up automatically. IMHO patchwork should be robust against these type of typos and convert them for the users (== your) convenience into 'Acked-by:' ... Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Hartkopp <oliver@hartkopp.net> Date: Wed, 10 Jun 2009 10:22:10 +0200 > David Miller wrote: >> Otherwise >> patchwork doesn't pick them up automatically. > > IMHO patchwork should be robust against these type of typos and convert them > for the users (== your) convenience into 'Acked-by:' ... We really can't expect patchwork to look for every conceivable malignment of the various reviewer tags. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 10 Jun 2009 01:10:27 -0700 (PDT) David Miller <davem@davemloft.net> wrote: > From: Oliver Hartkopp <oliver@hartkopp.net> > Date: Mon, 08 Jun 2009 15:24:58 +0200 > > > Acked-By: Oliver Hartkopp <oliver@hartkopp.net> > > Please don't capitalize the "By" in "Acked-By". Otherwise > patchwork doesn't pick them up automatically. Dave. We have computers to do all the silly tedious pointless jobs in life, please just fix the scripts. Alan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 2009-06-10 10:10, David Miller wrote: >> Acked-By: Oliver Hartkopp <oliver@hartkopp.net> > >Please don't capitalize the "By" in "Acked-By". Otherwise >patchwork doesn't pick them up automatically. Is not that a patchwork bug then that should be reported? Or a missing feature to checkpatch? Do we need a CodingStyle for SOBs now too, just for that? (Personally I prefer lowercase after the dash too, but I wanted to illuminate all sides of the box.) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David, > We really can't expect patchwork to look for every conceivable > malignment of the various reviewer tags. No, but we could probably be more tolerant about capitalisation. Any objections about ignoring case completely? I have a patch ready to push... Cheers, Jeremy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jeremy Kerr <jk@ozlabs.org> Date: Wed, 10 Jun 2009 22:41:47 +1000 > David, > >> We really can't expect patchwork to look for every conceivable >> malignment of the various reviewer tags. > > No, but we could probably be more tolerant about capitalisation. Any > objections about ignoring case completely? I have a patch ready to > push... Sure, sounds fine to me. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/can/af_can.c b/net/can/af_can.c index 10f0528..e733725 100644 --- a/net/can/af_can.c +++ b/net/can/af_can.c @@ -903,6 +903,8 @@ static __exit void can_exit(void) } spin_unlock(&can_rcvlists_lock); + rcu_barrier(); /* Wait for completion of call_rcu()'s */ + kmem_cache_destroy(rcv_cache); }
This module uses rcu_call() thus it should use rcu_barrier() on module unload. Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk> --- net/can/af_can.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html