| Message ID | 547367DF.5060706@kernel.dk |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Nov 24, 2014 at 10:16:15AM -0700, Jens Axboe wrote: > On 11/24/2014 09:22 AM, Paul E. McKenney wrote: > >On Mon, Nov 24, 2014 at 08:35:46AM -0700, Jens Axboe wrote: > >>On 11/24/2014 01:21 AM, Christoph Hellwig wrote: > >>>On Fri, Nov 21, 2014 at 02:56:00PM -0500, David Miller wrote: > >>>>I would suggest looking into the possibility that we allocate the memory > >>>>using the count of valid cpus, rather than the largest cpu number. > >>>> > >>>>That's a common error that runs into problems with discontiguous > >>>>cpu numbering like Sparc sometimes has. > >>> > >>>Yes, that does look like the case. Do you have a good trick on how > >>>to allocate a map for the highest possible cpu number without first > >>>iterating the cpu map? I couldn't find something that looks like a > >>>highest_possible_cpu() helper. > >> > >>Honestly I think that num_posible_cpus() should return the max of > >>number of CPUs (weigt), and the highest numbered CPU. It's a pain in > >>the butt to handle this otherwise. > > > >Hear, hear!!! That would make my life easier, and would make this sort > >of problem much less likely to occur! > > How about this one? Works for me! (Just for the record, as far as I know, this doesn't matter for RCU, which already uses nr_cpu_ids.) Thanx, Paul > -- > Jens Axboe > > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h > index 0a9a6da21e74..db21f68aaef0 100644 > --- a/include/linux/cpumask.h > +++ b/include/linux/cpumask.h > @@ -83,7 +83,13 @@ extern const struct cpumask *const cpu_active_mask; > > #if NR_CPUS > 1 > #define num_online_cpus() cpumask_weight(cpu_online_mask) > -#define num_possible_cpus() cpumask_weight(cpu_possible_mask) > +/* > + * For platforms with discontig CPU numbering, the weight of the possible > + * bitmask may be less than the highest numbered CPU. Return the max of > + * the two, to prevent accidentical bugs. > + */ > +#define num_possible_cpus() \ > + max_t(unsigned int, cpumask_weight(cpu_possible_mask), nr_cpu_ids) > #define num_present_cpus() cpumask_weight(cpu_present_mask) > #define num_active_cpus() cpumask_weight(cpu_active_mask) > #define cpu_online(cpu) cpumask_test_cpu((cpu), cpu_online_mask) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/24/2014 10:31 AM, Paul E. McKenney wrote: > On Mon, Nov 24, 2014 at 10:16:15AM -0700, Jens Axboe wrote: >> On 11/24/2014 09:22 AM, Paul E. McKenney wrote: >>> On Mon, Nov 24, 2014 at 08:35:46AM -0700, Jens Axboe wrote: >>>> On 11/24/2014 01:21 AM, Christoph Hellwig wrote: >>>>> On Fri, Nov 21, 2014 at 02:56:00PM -0500, David Miller wrote: >>>>>> I would suggest looking into the possibility that we allocate the memory >>>>>> using the count of valid cpus, rather than the largest cpu number. >>>>>> >>>>>> That's a common error that runs into problems with discontiguous >>>>>> cpu numbering like Sparc sometimes has. >>>>> >>>>> Yes, that does look like the case. Do you have a good trick on how >>>>> to allocate a map for the highest possible cpu number without first >>>>> iterating the cpu map? I couldn't find something that looks like a >>>>> highest_possible_cpu() helper. >>>> >>>> Honestly I think that num_posible_cpus() should return the max of >>>> number of CPUs (weigt), and the highest numbered CPU. It's a pain in >>>> the butt to handle this otherwise. >>> >>> Hear, hear!!! That would make my life easier, and would make this sort >>> of problem much less likely to occur! >> >> How about this one? > > Works for me! Thanks! I'll add an appropriate comment and send it out for review. > (Just for the record, as far as I know, this doesn't matter for RCU, > which already uses nr_cpu_ids.) Was that done after hitting something like this? Meelis, can you check if it fixes your issue?
On Mon, Nov 24, 2014 at 10:33:37AM -0700, Jens Axboe wrote: > On 11/24/2014 10:31 AM, Paul E. McKenney wrote: > >On Mon, Nov 24, 2014 at 10:16:15AM -0700, Jens Axboe wrote: > >>On 11/24/2014 09:22 AM, Paul E. McKenney wrote: > >>>On Mon, Nov 24, 2014 at 08:35:46AM -0700, Jens Axboe wrote: > >>>>On 11/24/2014 01:21 AM, Christoph Hellwig wrote: > >>>>>On Fri, Nov 21, 2014 at 02:56:00PM -0500, David Miller wrote: > >>>>>>I would suggest looking into the possibility that we allocate the memory > >>>>>>using the count of valid cpus, rather than the largest cpu number. > >>>>>> > >>>>>>That's a common error that runs into problems with discontiguous > >>>>>>cpu numbering like Sparc sometimes has. > >>>>> > >>>>>Yes, that does look like the case. Do you have a good trick on how > >>>>>to allocate a map for the highest possible cpu number without first > >>>>>iterating the cpu map? I couldn't find something that looks like a > >>>>>highest_possible_cpu() helper. > >>>> > >>>>Honestly I think that num_posible_cpus() should return the max of > >>>>number of CPUs (weigt), and the highest numbered CPU. It's a pain in > >>>>the butt to handle this otherwise. > >>> > >>>Hear, hear!!! That would make my life easier, and would make this sort > >>>of problem much less likely to occur! > >> > >>How about this one? > > > >Works for me! > > Thanks! I'll add an appropriate comment and send it out for review. > > >(Just for the record, as far as I know, this doesn't matter for RCU, > >which already uses nr_cpu_ids.) > > Was that done after hitting something like this? Nope. It was done after people started griping about RCU's older habit of sizing its data structures based on NR_CPUS. Something about distros cranking NR_CPUS up to 1024 and beyond. ;-) Thanx, Paul > Meelis, can you check if it fixes your issue? > > > -- > Jens Axboe > -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jens Axboe <axboe@kernel.dk> Date: Mon, 24 Nov 2014 10:16:15 -0700 > How about this one? The "num" in num_possible_cpus() means a count, as in how many are there. It doesn't mean largest ID of members of set X, which is what you are asking for. Even worse, having num_online_cpus() and num_possible_cpus() count from a different perspective is really confusing. Usually when people want a per-cpu thing they allocate percpu memory which hides all of these details, why don't you allocate the map as dynamic per-cpu memory? It will also do the NUMA node local allocations for you as well. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/24/2014 02:56 PM, David Miller wrote: > From: Jens Axboe <axboe@kernel.dk> > Date: Mon, 24 Nov 2014 10:16:15 -0700 > >> How about this one? > > The "num" in num_possible_cpus() means a count, as in how many are > there. > > It doesn't mean largest ID of members of set X, which is what you > are asking for. > > Even worse, having num_online_cpus() and num_possible_cpus() count > from a different perspective is really confusing. Not disagreeing with you... Personally I don't care too much, I can just work-around this in blk-mq. I'm more worried about others reimplementing the same bugs later. And yes, the fact that's it's the weight of the bitmap is exactly the problem, as we have no good way of saying "allocate me this array indexed by cpu count", since we don't know the numbers of the CPUs. This isn't a problem on x86 (or on anything but sparc64, as far as I'm aware), since the weight and highest CPU count are one and the same. > Usually when people want a per-cpu thing they allocate percpu > memory which hides all of these details, why don't you allocate > the map as dynamic per-cpu memory? > > It will also do the NUMA node local allocations for you as well. The allocation is node affine currently. This isn't per-cpu storage, for the per-cpu storage blk-mq uses the normal per-cpu primitives for alloctions. It's just a small array mapping a cpu number to a hardware queue number. It's read-only storage, after it has been updated. I'll just updated blk-mq to use nr_cpu_ids and be done with it.
From: Jens Axboe <axboe@kernel.dk> Date: Mon, 24 Nov 2014 15:01:55 -0700 > I'll just updated blk-mq to use nr_cpu_ids and be done with it. Wow, a grep on nr_cpu_ids gets a lot of hits on people allocating just these kinds of tables :) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/24/2014 03:09 PM, David Miller wrote: > From: Jens Axboe <axboe@kernel.dk> > Date: Mon, 24 Nov 2014 15:01:55 -0700 > >> I'll just updated blk-mq to use nr_cpu_ids and be done with it. > > Wow, a grep on nr_cpu_ids gets a lot of hits on people allocating just > these kinds of tables :) Yep! It'd be nicer to export a helper for that, I bet it's just one of those things that people started using because it's there and there are no functions for getting this information.
> > > > Yes, that does look like the case. Do you have a good trick on how > > > > to allocate a map for the highest possible cpu number without first > > > > iterating the cpu map? I couldn't find something that looks like a > > > > highest_possible_cpu() helper. > > > > > > Honestly I think that num_posible_cpus() should return the max of > > > number of CPUs (weigt), and the highest numbered CPU. It's a pain in > > > the butt to handle this otherwise. > > > > Hear, hear!!! That would make my life easier, and would make this sort > > of problem much less likely to occur! > > How about this one? It make the machine work.
From: mroos@linux.ee Date: Tue, 25 Nov 2014 00:23:20 +0200 (EET) >> > > > Yes, that does look like the case. Do you have a good trick on how >> > > > to allocate a map for the highest possible cpu number without first >> > > > iterating the cpu map? I couldn't find something that looks like a >> > > > highest_possible_cpu() helper. >> > > >> > > Honestly I think that num_posible_cpus() should return the max of >> > > number of CPUs (weigt), and the highest numbered CPU. It's a pain in >> > > the butt to handle this otherwise. >> > >> > Hear, hear!!! That would make my life easier, and would make this sort >> > of problem much less likely to occur! >> >> How about this one? > > It make the machine work. Thanks for testing! -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 24 Nov 2014, David Miller wrote: > From: mroos@linux.ee > Date: Tue, 25 Nov 2014 00:23:20 +0200 (EET) > > >> > > > Yes, that does look like the case. Do you have a good trick on how > >> > > > to allocate a map for the highest possible cpu number without first > >> > > > iterating the cpu map? I couldn't find something that looks like a > >> > > > highest_possible_cpu() helper. > >> > > > >> > > Honestly I think that num_posible_cpus() should return the max of > >> > > number of CPUs (weigt), and the highest numbered CPU. It's a pain in > >> > > the butt to handle this otherwise. > >> > > >> > Hear, hear!!! That would make my life easier, and would make this sort > >> > of problem much less likely to occur! > >> > >> How about this one? > > > > It make the machine work. > > Thanks for testing! > What's the status of this fix? It is still not applied on yesterdays 3.19.0-rc6-00105-gc59c961 git...
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index 0a9a6da21e74..db21f68aaef0 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -83,7 +83,13 @@ extern const struct cpumask *const cpu_active_mask; #if NR_CPUS > 1 #define num_online_cpus() cpumask_weight(cpu_online_mask) -#define num_possible_cpus() cpumask_weight(cpu_possible_mask) +/* + * For platforms with discontig CPU numbering, the weight of the possible + * bitmask may be less than the highest numbered CPU. Return the max of + * the two, to prevent accidentical bugs. + */ +#define num_possible_cpus() \ + max_t(unsigned int, cpumask_weight(cpu_possible_mask), nr_cpu_ids) #define num_present_cpus() cpumask_weight(cpu_present_mask) #define num_active_cpus() cpumask_weight(cpu_active_mask) #define cpu_online(cpu) cpumask_test_cpu((cpu), cpu_online_mask)