Re: [PATCH v2 4/6] block: switch to per-cpu in-flight counters

[Jens Axboe <axboe@kernel.dk>]

On 12/5/18 10:49 AM, Mike Snitzer wrote:
> On Wed, Dec 05 2018 at 12:30pm -0500,
> Jens Axboe <axboe@kernel.dk> wrote:
> 
>> On 11/30/18 3:22 PM, Mike Snitzer wrote:
>>> diff --git a/block/genhd.c b/block/genhd.c
>>> index cdf174d7d329..d4c9dd65def6 100644
>>> --- a/block/genhd.c
>>> +++ b/block/genhd.c
>>> @@ -45,53 +45,76 @@ static void disk_add_events(struct gendisk *disk);
>>>  static void disk_del_events(struct gendisk *disk);
>>>  static void disk_release_events(struct gendisk *disk);
>>>  
>>> -void part_inc_in_flight(struct request_queue *q, struct hd_struct *part, int rw)
>>> +void part_inc_in_flight(struct request_queue *q, int cpu, struct hd_struct *part, int rw)
>>>  {
>>>  	if (queue_is_mq(q))
>>>  		return;
>>>  
>>> -	atomic_inc(&part->in_flight[rw]);
>>> +	local_inc(&per_cpu_ptr(part->dkstats, cpu)->in_flight[rw]);
>>
>> I mentioned this in a previous email, but why isn't this just using
>> this_cpu_inc?
> 
> I responded to your earlier question on this point but, Mikulas just
> extended the existing percpu struct disk_stats and he is using local_t
> for reasons detailed in this patch's header:
> 
>     We use the local-atomic type local_t, so that if part_inc_in_flight or
>     part_dec_in_flight is reentrantly called from an interrupt, the value will
>     be correct.
> 
>     The other counters could be corrupted due to reentrant interrupt, but the
>     corruption only results in slight counter skew - the in_flight counter
>     must be exact, so it needs local_t.

Gotcha, make sense.

>> There's also no need to pass in the cpu, if we're not running with
>> preempt disabled already we have a problem. 
> 
> Why should this be any different than the part_stat_* interfaces?
> __part_stat_add(), part_stat_read(), etc also use
> per_cpu_ptr((part)->dkstats, (cpu) accessors.

Maybe audit which ones actually need it? To answer the specific question,
it's silly to pass in the cpu, if we're pinned already. That's true
both programatically, but also for someone reading the code.


-- 
Jens Axboe