| Message ID | 20130623134331.19094.80396.stgit@srivatsabhat.in.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sun, Jun 23, 2013 at 07:13:33PM +0530, Srivatsa S. Bhat wrote: > Once stop_machine() is gone from the CPU offline path, we won't be able > to depend on disabling preemption to prevent CPUs from going offline > from under us. > > Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going > offline, while invoking from atomic context. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: devel@driverdev.osuosl.org > Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > --- > > drivers/staging/octeon/ethernet-rx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c > index 34afc16..8588b4d 100644 > --- a/drivers/staging/octeon/ethernet-rx.c > +++ b/drivers/staging/octeon/ethernet-rx.c > @@ -36,6 +36,7 @@ > #include <linux/prefetch.h> > #include <linux/ratelimit.h> > #include <linux/smp.h> > +#include <linux/cpu.h> > #include <linux/interrupt.h> > #include <net/dst.h> > #ifdef CONFIG_XFRM > @@ -97,6 +98,7 @@ static void cvm_oct_enable_one_cpu(void) > return; > > /* ... if a CPU is available, Turn on NAPI polling for that CPU. */ > + get_online_cpus_atomic(); > for_each_online_cpu(cpu) { > if (!cpu_test_and_set(cpu, core_state.cpu_state)) { > v = smp_call_function_single(cpu, cvm_oct_enable_napi, > @@ -106,6 +108,7 @@ static void cvm_oct_enable_one_cpu(void) > break; > } > } > + put_online_cpus_atomic(); Does this driver really need to be doing this in the first place? If so, why? The majority of network drivers don't, why is this one "special"? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2013 11:47 PM, Greg Kroah-Hartman wrote: > On Sun, Jun 23, 2013 at 07:13:33PM +0530, Srivatsa S. Bhat wrote: >> Once stop_machine() is gone from the CPU offline path, we won't be able >> to depend on disabling preemption to prevent CPUs from going offline >> from under us. >> >> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going >> offline, while invoking from atomic context. >> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: devel@driverdev.osuosl.org >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> >> --- >> >> drivers/staging/octeon/ethernet-rx.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c >> index 34afc16..8588b4d 100644 >> --- a/drivers/staging/octeon/ethernet-rx.c >> +++ b/drivers/staging/octeon/ethernet-rx.c >> @@ -36,6 +36,7 @@ >> #include <linux/prefetch.h> >> #include <linux/ratelimit.h> >> #include <linux/smp.h> >> +#include <linux/cpu.h> >> #include <linux/interrupt.h> >> #include <net/dst.h> >> #ifdef CONFIG_XFRM >> @@ -97,6 +98,7 @@ static void cvm_oct_enable_one_cpu(void) >> return; >> >> /* ... if a CPU is available, Turn on NAPI polling for that CPU. */ >> + get_online_cpus_atomic(); >> for_each_online_cpu(cpu) { >> if (!cpu_test_and_set(cpu, core_state.cpu_state)) { >> v = smp_call_function_single(cpu, cvm_oct_enable_napi, >> @@ -106,6 +108,7 @@ static void cvm_oct_enable_one_cpu(void) >> break; >> } >> } >> + put_online_cpus_atomic(); > > Does this driver really need to be doing this in the first place? If > so, why? The majority of network drivers don't, why is this one > "special"? > Honestly, I don't know. Let's CC the author of that code (David Daney). I wonder why get_maintainer.pl didn't generate his name for this file, even though the entire file is almost made up of his commits alone! Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-06-24 at 00:25 +0530, Srivatsa S. Bhat wrote: > On 06/23/2013 11:47 PM, Greg Kroah-Hartman wrote: > > On Sun, Jun 23, 2013 at 07:13:33PM +0530, Srivatsa S. Bhat wrote: [] > >> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c [] > Honestly, I don't know. Let's CC the author of that code (David Daney). > I wonder why get_maintainer.pl didn't generate his name for this file, > even though the entire file is almost made up of his commits alone! Because by default, get_maintainer looks for a matching file entry in MAINTAINERS. Failing that, it looks at one year of git history. In this case, no work has been done on the file for quite awhile. --git-blame can be added to the get_maintainer.pl command line to look for % of authorship by line and commit count. Adding --git-blame can take a long time to run, that's why it's not on by default. Also, very old history can give invalid email addresses as people move around and email addresses decay. If you always want to find original authors, you could use a .get_maintainer.conf file with --git-blame in it. $ time ./scripts/get_maintainer.pl --git-blame -f drivers/staging/octeon/ethernet-tx.c Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:STAGING SUBSYSTEM,commits:4/16=25%) David Daney <ddaney@caviumnetworks.com> (authored lines:711/725=98%,commits:13/16=81%) Ralf Baechle <ralf@linux-mips.org> (commits:11/16=69%) Eric Dumazet <eric.dumazet@gmail.com> (commits:2/16=12%) Andrew Morton <akpm@linux-foundation.org> (commits:1/16=6%) devel@driverdev.osuosl.org (open list:STAGING SUBSYSTEM) linux-kernel@vger.kernel.org (open list) real 0m16.853s user 0m16.088s sys 0m0.444s -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/24/2013 12:47 AM, Joe Perches wrote: > On Mon, 2013-06-24 at 00:25 +0530, Srivatsa S. Bhat wrote: >> On 06/23/2013 11:47 PM, Greg Kroah-Hartman wrote: >>> On Sun, Jun 23, 2013 at 07:13:33PM +0530, Srivatsa S. Bhat wrote: > [] >>>> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c > [] >> Honestly, I don't know. Let's CC the author of that code (David Daney). >> I wonder why get_maintainer.pl didn't generate his name for this file, >> even though the entire file is almost made up of his commits alone! > > Because by default, get_maintainer looks for a matching > file entry in MAINTAINERS. Failing that, it looks at > one year of git history. In this case, no work has been > done on the file for quite awhile. > > --git-blame can be added to the get_maintainer.pl command > line to look for % of authorship by line and commit count. > > Adding --git-blame can take a long time to run, that's why > it's not on by default. Also, very old history can give > invalid email addresses as people move around and email > addresses decay. > > If you always want to find original authors, you could > use a .get_maintainer.conf file with --git-blame in it. > > $ time ./scripts/get_maintainer.pl --git-blame -f drivers/staging/octeon/ethernet-tx.c > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:STAGING SUBSYSTEM,commits:4/16=25%) > David Daney <ddaney@caviumnetworks.com> (authored lines:711/725=98%,commits:13/16=81%) > Ralf Baechle <ralf@linux-mips.org> (commits:11/16=69%) > Eric Dumazet <eric.dumazet@gmail.com> (commits:2/16=12%) > Andrew Morton <akpm@linux-foundation.org> (commits:1/16=6%) > devel@driverdev.osuosl.org (open list:STAGING SUBSYSTEM) > linux-kernel@vger.kernel.org (open list) > > real 0m16.853s > user 0m16.088s > sys 0m0.444s > > Oh, ok.. Thanks for the explanation and the tip! Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2013 11:55 AM, Srivatsa S. Bhat wrote: > On 06/23/2013 11:47 PM, Greg Kroah-Hartman wrote: >> On Sun, Jun 23, 2013 at 07:13:33PM +0530, Srivatsa S. Bhat wrote: >>> Once stop_machine() is gone from the CPU offline path, we won't be able >>> to depend on disabling preemption to prevent CPUs from going offline >>> from under us. >>> >>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going >>> offline, while invoking from atomic context. >>> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: devel@driverdev.osuosl.org >>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> >>> --- >>> >>> drivers/staging/octeon/ethernet-rx.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c >>> index 34afc16..8588b4d 100644 >>> --- a/drivers/staging/octeon/ethernet-rx.c >>> +++ b/drivers/staging/octeon/ethernet-rx.c >>> @@ -36,6 +36,7 @@ >>> #include <linux/prefetch.h> >>> #include <linux/ratelimit.h> >>> #include <linux/smp.h> >>> +#include <linux/cpu.h> >>> #include <linux/interrupt.h> >>> #include <net/dst.h> >>> #ifdef CONFIG_XFRM >>> @@ -97,6 +98,7 @@ static void cvm_oct_enable_one_cpu(void) >>> return; >>> >>> /* ... if a CPU is available, Turn on NAPI polling for that CPU. */ >>> + get_online_cpus_atomic(); >>> for_each_online_cpu(cpu) { >>> if (!cpu_test_and_set(cpu, core_state.cpu_state)) { >>> v = smp_call_function_single(cpu, cvm_oct_enable_napi, >>> @@ -106,6 +108,7 @@ static void cvm_oct_enable_one_cpu(void) >>> break; >>> } >>> } >>> + put_online_cpus_atomic(); >> >> Does this driver really need to be doing this in the first place? If >> so, why? The majority of network drivers don't, why is this one >> "special"? It depends on your definition of "need". The current driver receives packets from *all* network ports into a single queue (in OCTEON speak this queue is called a POW group). Under high packet rates, the CPU time required to process the packets may exceed the capabilities of a single CPU. In order to increase throughput beyond the single CPU limited rate, we bring more than one CPUs into play for NAPI receive. The code being patched here is part of the logic that controls which CPUs are used for NAPI receive. Just for the record: Yes I know that doing this may lead to packet reordering when doing forwarding. A further question that wasn't asked is: Will the code work at all if a CPU is taken offline even if the race, the patch eliminates, is avoided? I doubt it. As far as the patch goes: Acked-by: David Daney <david.daney@cavium.com> David Daney >> > > Honestly, I don't know. Let's CC the author of that code (David Daney). > I wonder why get_maintainer.pl didn't generate his name for this file, > even though the entire file is almost made up of his commits alone! > > Regards, > Srivatsa S. Bhat > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c index 34afc16..8588b4d 100644 --- a/drivers/staging/octeon/ethernet-rx.c +++ b/drivers/staging/octeon/ethernet-rx.c @@ -36,6 +36,7 @@ #include <linux/prefetch.h> #include <linux/ratelimit.h> #include <linux/smp.h> +#include <linux/cpu.h> #include <linux/interrupt.h> #include <net/dst.h> #ifdef CONFIG_XFRM @@ -97,6 +98,7 @@ static void cvm_oct_enable_one_cpu(void) return; /* ... if a CPU is available, Turn on NAPI polling for that CPU. */ + get_online_cpus_atomic(); for_each_online_cpu(cpu) { if (!cpu_test_and_set(cpu, core_state.cpu_state)) { v = smp_call_function_single(cpu, cvm_oct_enable_napi, @@ -106,6 +108,7 @@ static void cvm_oct_enable_one_cpu(void) break; } } + put_online_cpus_atomic(); } static void cvm_oct_no_more_work(void)
Once stop_machine() is gone from the CPU offline path, we won't be able to depend on disabling preemption to prevent CPUs from going offline from under us. Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline, while invoking from atomic context. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: devel@driverdev.osuosl.org Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- drivers/staging/octeon/ethernet-rx.c | 3 +++ 1 file changed, 3 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html