Another (ESP?) scsi blk-mq problem on sparc64

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.

/* If cpus are offline, map them to first hctx */
map = kzalloc_node(sizeof(*map) * num_possible_cpus(), GFP_KERNEL,
                                 set->numa_node);

is where it goes wrong, assuming Meelis has NR_CPUS set to something < 
14, which was the highest numbered CPU, iirc. A construct like:

map = alloc(num_possible_cpus());
for_each_possible_cpu(i)
     map[i] = ...

seems like the obvious way to do things, yet it's broken. And not broken 
on x86 where we'd find the issue pretty quickly, but on sparc64 where 
it'll take a lot longer to run into. It just violates the principle of 
least surprise, which is bad for an API.

That said, if there was a helper like Christoph suggested, it'd be a bit 
better. And perhaps we can't just modify num_possible_cpus(), we'd need 
num_*_cpus() for the other bitmaps, too. That might be breaking other 
things...

Meelis, can you try the attached?

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!

							Thanx, Paul

> /* If cpus are offline, map them to first hctx */
> map = kzalloc_node(sizeof(*map) * num_possible_cpus(), GFP_KERNEL,
>                                 set->numa_node);
> 
> is where it goes wrong, assuming Meelis has NR_CPUS set to something
> < 14, which was the highest numbered CPU, iirc. A construct like:
> 
> map = alloc(num_possible_cpus());
> for_each_possible_cpu(i)
>     map[i] = ...
> 
> seems like the obvious way to do things, yet it's broken. And not
> broken on x86 where we'd find the issue pretty quickly, but on
> sparc64 where it'll take a lot longer to run into. It just violates
> the principle of least surprise, which is bad for an API.
> 
> That said, if there was a helper like Christoph suggested, it'd be a
> bit better. And perhaps we can't just modify num_possible_cpus(),
> we'd need num_*_cpus() for the other bitmaps, too. That might be
> breaking other things...
> 
> Meelis, can you try the attached?
> 
> -- 
> Jens Axboe
> 

> diff --git a/block/blk-mq-cpumap.c b/block/blk-mq-cpumap.c
> index 1065d7c65fa1..15da9cc08f64 100644
> --- a/block/blk-mq-cpumap.c
> +++ b/block/blk-mq-cpumap.c
> @@ -87,10 +87,14 @@ int blk_mq_update_queue_map(unsigned int *map, unsigned int nr_queues)
> 
>  unsigned int *blk_mq_make_queue_map(struct blk_mq_tag_set *set)
>  {
> +	unsigned int max_cpus;
>  	unsigned int *map;
> 
> +	for_each_possible_cpu(max_cpus)
> +		;
> +
>  	/* If cpus are offline, map them to first hctx */
> -	map = kzalloc_node(sizeof(*map) * num_possible_cpus(), GFP_KERNEL,
> +	map = kzalloc_node(sizeof(*map) * max_cpus, GFP_KERNEL,
>  				set->numa_node);
>  	if (!map)
>  		return NULL;

--
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