| Message ID | 1328201363-13915-3-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Le jeudi 02 février 2012 à 16:49 +0000, Wei Liu a écrit : > Enables users to unload netback module. > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netback/common.h | 1 + > drivers/net/xen-netback/netback.c | 14 ++++++++++++++ > drivers/net/xen-netback/xenbus.c | 5 +++++ > 3 files changed, 20 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h > index 288b2f3..372c7f5 100644 > --- a/drivers/net/xen-netback/common.h > +++ b/drivers/net/xen-netback/common.h > @@ -126,6 +126,7 @@ void xenvif_get(struct xenvif *vif); > void xenvif_put(struct xenvif *vif); > > int xenvif_xenbus_init(void); > +void xenvif_xenbus_exit(void); > > int xenvif_schedulable(struct xenvif *vif); > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > index d11205f..3059684 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -1670,5 +1670,19 @@ failed_init: > > module_init(netback_init); > While reviewing this code, I can see current netback_init() is buggy. It assumes all online cpus xen_netbk_group_nr are numbered from 0 to xen_netbk_group_nr-1 This is not right. Instead of using : xen_netbk = vzalloc(sizeof(struct xen_netbk) * xen_netbk_group_nr); You should use a percpu variable to get proper NUMA properties. And instead of looping like : for (group = 0; group < xen_netbk_group_nr; group++) { You must use : for_each_online_cpu(cpu) { ... } [ and also use kthread_create_on_node() instead of kthread_create() ] -- 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 Thu, 2012-02-02 at 17:08 +0000, Eric Dumazet wrote: > Le jeudi 02 février 2012 à 16:49 +0000, Wei Liu a écrit : > > Enables users to unload netback module. > > > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > drivers/net/xen-netback/common.h | 1 + > > drivers/net/xen-netback/netback.c | 14 ++++++++++++++ > > drivers/net/xen-netback/xenbus.c | 5 +++++ > > 3 files changed, 20 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h > > index 288b2f3..372c7f5 100644 > > --- a/drivers/net/xen-netback/common.h > > +++ b/drivers/net/xen-netback/common.h > > @@ -126,6 +126,7 @@ void xenvif_get(struct xenvif *vif); > > void xenvif_put(struct xenvif *vif); > > > > int xenvif_xenbus_init(void); > > +void xenvif_xenbus_exit(void); > > > > int xenvif_schedulable(struct xenvif *vif); > > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > > index d11205f..3059684 100644 > > --- a/drivers/net/xen-netback/netback.c > > +++ b/drivers/net/xen-netback/netback.c > > @@ -1670,5 +1670,19 @@ failed_init: > > > > module_init(netback_init); > > > > While reviewing this code, I can see current netback_init() is buggy. > > It assumes all online cpus xen_netbk_group_nr are numbered from 0 to > xen_netbk_group_nr-1 > > This is not right. > You're right about this. But this part is destined to get wiped out (in the very near future?) -- see following patches. So I don't think it is worthy to fix this. Wei. > Instead of using : > > xen_netbk = vzalloc(sizeof(struct xen_netbk) * xen_netbk_group_nr); > > You should use a percpu variable to get proper NUMA properties. > > And instead of looping like : > > for (group = 0; group < xen_netbk_group_nr; group++) { > > You must use : > > for_each_online_cpu(cpu) { > ... > } > > [ and also use kthread_create_on_node() instead of kthread_create() ] > > > -- 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
Le jeudi 02 février 2012 à 17:28 +0000, Wei Liu a écrit : > You're right about this. > > But this part is destined to get wiped out (in the very near future?) -- > see following patches. So I don't think it is worthy to fix this. > Before adding new bugs, you must fix previous ones. Do you think stable teams will do the backport of all your upcoming patches ? -- 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 Thu, 2012-02-02 at 17:48 +0000, Eric Dumazet wrote: > Le jeudi 02 février 2012 à 17:28 +0000, Wei Liu a écrit : > > > You're right about this. > > > > But this part is destined to get wiped out (in the very near future?) -- > > see following patches. So I don't think it is worthy to fix this. > > > > Before adding new bugs, you must fix previous ones. I've never heard of this requirement before! It's a wonder anyone ever gets anything done. Anyway, I think it would be reasonable to just remove the kthread_bind call from this loop. We don't actually want/need a thread per online CPU in any strict sense, we just want there to be some number of worker threads available and ~numcpus at start of day is a good enough approximation for that number. There have been patches floating around in the past which make the number of groups a module parameter which would also be a reasonable thing to dig out if we weren't just about to remove all this code anyway. So removing the kthread_bind is good enough for the short term, and for stable if people feel that is necessary, and we can continue in mainline with the direction Wei's patches are taking us. Ian. -- 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
Le jeudi 02 février 2012 à 19:59 +0000, Ian Campbell a écrit : > On Thu, 2012-02-02 at 17:48 +0000, Eric Dumazet wrote: > > Le jeudi 02 février 2012 à 17:28 +0000, Wei Liu a écrit : > > > > > You're right about this. > > > > > > But this part is destined to get wiped out (in the very near future?) -- > > > see following patches. So I don't think it is worthy to fix this. > > > > > > > Before adding new bugs, you must fix previous ones. > > I've never heard of this requirement before! It's a wonder anyone ever > gets anything done. > > Anyway, I think it would be reasonable to just remove the kthread_bind > call from this loop. We don't actually want/need a thread per online CPU > in any strict sense, we just want there to be some number of worker > threads available and ~numcpus at start of day is a good enough > approximation for that number. There have been patches floating around > in the past which make the number of groups a module parameter which > would also be a reasonable thing to dig out if we weren't just about to > remove all this code anyway. > > So removing the kthread_bind is good enough for the short term, and for > stable if people feel that is necessary, and we can continue in mainline > with the direction Wei's patches are taking us. > That sounds a right fix. Why do think its not reasonable that I ask a bug fix ? Next time, dont bother send patches for review if you dont want reviewers. -- 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
Le jeudi 02 février 2012 à 21:34 +0100, Eric Dumazet a écrit : > That sounds a right fix. > > Why do think its not reasonable that I ask a bug fix ? > > Next time, dont bother send patches for review if you dont want > reviewers. FYI, here the trace you can get right now with this bug : [ 1180.114682] WARNING: at arch/x86/kernel/smp.c:120 native_smp_send_reschedule+0x5b/0x60() [ 1180.114685] Hardware name: ProLiant BL460c G6 [ 1180.114686] Modules linked in: ipmi_devintf nfsd exportfs ipmi_si hpilo bnx2x crc32c libcrc32c mdio [last unloaded: scsi_wait_scan] [ 1180.114697] Pid: 7, comm: migration/1 Not tainted 3.3.0-rc2+ #609 [ 1180.114699] Call Trace: [ 1180.114701] <IRQ> [<ffffffff81037adf>] warn_slowpath_common+0x7f/0xc0 [ 1180.114708] [<ffffffff81037b3a>] warn_slowpath_null+0x1a/0x20 [ 1180.114711] [<ffffffff8101ecfb>] native_smp_send_reschedule+0x5b/0x60 [ 1180.114715] [<ffffffff810744ec>] trigger_load_balance+0x28c/0x370 [ 1180.114720] [<ffffffff8106be14>] scheduler_tick+0x114/0x160 [ 1180.114724] [<ffffffff8104956e>] update_process_times+0x6e/0x90 [ 1180.114729] [<ffffffff8108c614>] tick_sched_timer+0x64/0xc0 [ 1180.114733] [<ffffffff8105fe54>] __run_hrtimer+0x84/0x1f0 [ 1180.114736] [<ffffffff8108c5b0>] ? tick_nohz_handler+0xf0/0xf0 [ 1180.114739] [<ffffffff8103f271>] ? __do_softirq+0x101/0x240 [ 1180.114742] [<ffffffff81060783>] hrtimer_interrupt+0xf3/0x220 [ 1180.114747] [<ffffffff810a90b0>] ? queue_stop_cpus_work+0x100/0x100 [ 1180.114751] [<ffffffff8171ee09>] smp_apic_timer_interrupt+0x69/0x99 [ 1180.114754] [<ffffffff8171dd4b>] apic_timer_interrupt+0x6b/0x70 [ 1180.114756] <EOI> [<ffffffff810a9143>] ? stop_machine_cpu_stop+0x93/0xc0 [ 1180.114761] [<ffffffff810a8da7>] cpu_stopper_thread+0xd7/0x1a0 [ 1180.114766] [<ffffffff81713dc7>] ? __schedule+0x3a7/0x7e0 [ 1180.114768] [<ffffffff81064058>] ? __wake_up_common+0x58/0x90 [ 1180.114771] [<ffffffff810a8cd0>] ? cpu_stop_signal_done+0x40/0x40 [ 1180.114773] [<ffffffff8105b5c3>] kthread+0x93/0xa0 [ 1180.114776] [<ffffffff8171e594>] kernel_thread_helper+0x4/0x10 [ 1180.114779] [<ffffffff8105b530>] ? kthread_freezable_should_stop+0x80/0x80 [ 1180.114781] [<ffffffff8171e590>] ? gs_change+0xb/0xb -- 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 Thu, 2012-02-02 at 20:34 +0000, Eric Dumazet wrote: > Le jeudi 02 février 2012 à 19:59 +0000, Ian Campbell a écrit : > > On Thu, 2012-02-02 at 17:48 +0000, Eric Dumazet wrote: > > > Le jeudi 02 février 2012 à 17:28 +0000, Wei Liu a écrit : > > > > > > > You're right about this. > > > > > > > > But this part is destined to get wiped out (in the very near future?) -- > > > > see following patches. So I don't think it is worthy to fix this. > > > > > > > > > > Before adding new bugs, you must fix previous ones. > > > > I've never heard of this requirement before! It's a wonder anyone ever > > gets anything done. > > > > Anyway, I think it would be reasonable to just remove the kthread_bind > > call from this loop. We don't actually want/need a thread per online CPU > > in any strict sense, we just want there to be some number of worker > > threads available and ~numcpus at start of day is a good enough > > approximation for that number. There have been patches floating around > > in the past which make the number of groups a module parameter which > > would also be a reasonable thing to dig out if we weren't just about to > > remove all this code anyway. > > > > So removing the kthread_bind is good enough for the short term, and for > > stable if people feel that is necessary, and we can continue in mainline > > with the direction Wei's patches are taking us. > > > > That sounds a right fix. > > Why do think its not reasonable that I ask a bug fix ? I don't think it is at all unreasonable to ask for bug fixes but in this case Wei's series is removing the code in question (which would also undoubtedly fix the bug). As it happens the fix turns out to be simple but if it were complex I would perhaps have disagreed more strongly about spending effort fixing code that is removed 2 patches later, although obviously that would have depended on the specifics of the fix in that case. > Next time, dont bother send patches for review if you dont want > reviewers. The review which you are doing is certainly very much appreciated, I'm sorry if my disagreement over this one point gave/gives the impression that it is not. Ian. -- 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 Thu, Feb 2, 2012 at 3:50 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Thu, 2012-02-02 at 20:34 +0000, Eric Dumazet wrote: [...] > > I don't think it is at all unreasonable to ask for bug fixes but in this > case Wei's series is removing the code in question (which would also > undoubtedly fix the bug). > > As it happens the fix turns out to be simple but if it were complex I > would perhaps have disagreed more strongly about spending effort fixing > code that is removed 2 patches later, although obviously that would have > depended on the specifics of the fix in that case. Lots of people are relying on git bisect. If you introduce build failures or known bugs into any point in history, you take away from the value in git bisect. Sure, it happens by accident, but it shouldn't ever be done knowingly. Paul. > >> Next time, dont bother send patches for review if you dont want >> reviewers. > > The review which you are doing is certainly very much appreciated, I'm > sorry if my disagreement over this one point gave/gives the impression > that it is not. > > Ian. > > -- > 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 Thu, 2012-02-02 at 22:52 +0000, Paul Gortmaker wrote: > On Thu, Feb 2, 2012 at 3:50 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Thu, 2012-02-02 at 20:34 +0000, Eric Dumazet wrote: > > [...] > > > > > I don't think it is at all unreasonable to ask for bug fixes but in this > > case Wei's series is removing the code in question (which would also > > undoubtedly fix the bug). > > > > As it happens the fix turns out to be simple but if it were complex I > > would perhaps have disagreed more strongly about spending effort fixing > > code that is removed 2 patches later, although obviously that would have > > depended on the specifics of the fix in that case. > > Lots of people are relying on git bisect. If you introduce build failures > or known bugs into any point in history, you take away from the value > in git bisect. Sure, it happens by accident, but it shouldn't ever be > done knowingly. Sure. In this case the bug has been there since 2.6.39, it isn't introduced by this series. Ian. -- 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
Le vendredi 03 février 2012 à 06:38 +0000, Ian Campbell a écrit : > On Thu, 2012-02-02 at 22:52 +0000, Paul Gortmaker wrote: > > On Thu, Feb 2, 2012 at 3:50 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > On Thu, 2012-02-02 at 20:34 +0000, Eric Dumazet wrote: > > > > [...] > > > > > > > > I don't think it is at all unreasonable to ask for bug fixes but in this > > > case Wei's series is removing the code in question (which would also > > > undoubtedly fix the bug). > > > > > > As it happens the fix turns out to be simple but if it were complex I > > > would perhaps have disagreed more strongly about spending effort fixing > > > code that is removed 2 patches later, although obviously that would have > > > depended on the specifics of the fix in that case. > > > > Lots of people are relying on git bisect. If you introduce build failures > > or known bugs into any point in history, you take away from the value > > in git bisect. Sure, it happens by accident, but it shouldn't ever be > > done knowingly. > > Sure. In this case the bug has been there since 2.6.39, it isn't > introduced by this series. > We are stuck right now with a bug introduced in 2.6.39, (IP redirects), and because fix was done in 3.1, we are unable to provide a fix fo stable 3.0 kernel. Something that takes 15 minutes to fix now, can take several days of work later. -- 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 Fri, 2012-02-03 at 07:25 +0000, Eric Dumazet wrote: > > We are stuck right now with a bug introduced in 2.6.39, (IP redirects), > and because fix was done in 3.1, we are unable to provide a fix fo > stable 3.0 kernel. > > Something that takes 15 minutes to fix now, can take several days of > work later. > > > You're right. Will stick Ian's patch in front of my next series. Thanks for your effort in reviewing. Wei. -- 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/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index 288b2f3..372c7f5 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -126,6 +126,7 @@ void xenvif_get(struct xenvif *vif); void xenvif_put(struct xenvif *vif); int xenvif_xenbus_init(void); +void xenvif_xenbus_exit(void); int xenvif_schedulable(struct xenvif *vif); diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index d11205f..3059684 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -1670,5 +1670,19 @@ failed_init: module_init(netback_init); +static void __exit netback_exit(void) +{ + int i; + xenvif_xenbus_exit(); + for (i = 0; i < xen_netbk_group_nr; i++) { + struct xen_netbk *netbk = &xen_netbk[i]; + del_timer_sync(&netbk->net_timer); + kthread_stop(netbk->task); + } + vfree(xen_netbk); + page_pool_destroy(); +} +module_exit(netback_exit); + MODULE_LICENSE("Dual BSD/GPL"); MODULE_ALIAS("xen-backend:vif"); diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 410018c..65d14f2 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -485,3 +485,8 @@ int xenvif_xenbus_init(void) { return xenbus_register_backend(&netback_driver); } + +void xenvif_xenbus_exit(void) +{ + return xenbus_unregister_driver(&netback_driver); +}