Re: [PATCH] block: add a new flag BLK_MQ_POLL_HYBRID_INVALID for hybrid poll

From: yuyufen <yuyufen@huawei.com>
To: Damien Le Moal <Damien.LeMoal@wdc.com>,
	Marcos Paulo de Souza <marcos.souza.org@gmail.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: [PATCH] block: add a new flag BLK_MQ_POLL_HYBRID_INVALID for hybrid poll
Date: Thu, 28 Feb 2019 18:53:45 +0800	[thread overview]
Message-ID: <f9595015-9fa4-f78c-d8b1-0e84699be1c7@huawei.com> (raw)
In-Reply-To: <BYAPR04MB581600BE4E6B945E3E52CD75E7740@BYAPR04MB5816.namprd04.prod.outlook.com>

On 2019/2/27 20:45, Damien Le Moal wrote:
> On 2019/02/27 5:38, yuyufen wrote:
>> On 2019/2/27 18:27, Marcos Paulo de Souza wrote:
>>> Hello Yufen,
>>>
>>> On 2/27/19 3:21 AM, Yufen Yu wrote:
>>>> For q->poll_nsec == -1, means using classic poll, not hybrid poll.
>>>> We introduce a new flag BLK_POLL_DELAY_INVALID to replace -1, which
>>>> may make code much easier to read.
>>> In the commit message you say BLK_POLL_DELAY_INVALID, but the code and
>>> the commit title use BLK_MQ_POLL_HYBRID_INVALID. Is this a typo?
>>>
>> Thanks a lot for pointing out this fault. What I want to
>> say is BLK_MQ_POLL_HYBRID_INVALID, not BLK_POLL_DELAY_INVALID.
>>
>> Yufen
>> Thanks
>>
>>> Thanks.
>>>
>>>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
>>>> ---
>>>>    block/blk-mq.c         | 4 ++--
>>>>    block/blk-sysfs.c      | 4 ++--
>>>>    include/linux/blkdev.h | 3 +++
>>>>    3 files changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>> index 9437a5eb07cf..8df9f6e97d2e 100644
>>>> --- a/block/blk-mq.c
>>>> +++ b/block/blk-mq.c
>>>> @@ -2857,7 +2857,7 @@ struct request_queue
>>>> *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>>>>        /*
>>>>         * Default to classic polling
>>>>         */
>>>> -    q->poll_nsec = -1;
>>>> +    q->poll_nsec = BLK_MQ_POLL_HYBRID_INVALID;
>>>>          blk_mq_init_cpu_queues(q, set->nr_hw_queues);
>>>>        blk_mq_add_queue_tag_set(set, q);
>>>> @@ -3389,7 +3389,7 @@ static bool blk_mq_poll_hybrid(struct
>>>> request_queue *q,
>>>>    {
>>>>        struct request *rq;
>>>>    -    if (q->poll_nsec == -1)
>>>> +    if (q->poll_nsec == BLK_MQ_POLL_HYBRID_INVALID)
>>>>            return false;
>>>>          if (!blk_qc_t_is_internal(cookie))
>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>>> index 590d1ef2f961..8775b898f96d 100644
>>>> --- a/block/blk-sysfs.c
>>>> +++ b/block/blk-sysfs.c
>>>> @@ -360,7 +360,7 @@ static ssize_t queue_poll_delay_show(struct
>>>> request_queue *q, char *page)
>>>>    {
>>>>        int val;
>>>>    -    if (q->poll_nsec == -1)
>>>> +    if (q->poll_nsec == BLK_MQ_POLL_HYBRID_INVALID)
>>>>            val = -1;
>>>>        else
>>>>            val = q->poll_nsec / 1000;
>>>> @@ -381,7 +381,7 @@ static ssize_t queue_poll_delay_store(struct
>>>> request_queue *q, const char *page,
>>>>            return err;
>>>>          if (val == -1)
>>>> -        q->poll_nsec = -1;
>>>> +        q->poll_nsec = BLK_MQ_POLL_HYBRID_INVALID;
>>>>        else
>>>>            q->poll_nsec = val * 1000;
>>>>    diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>> index 338604dff7d0..2102447b67d9 100644
>>>> --- a/include/linux/blkdev.h
>>>> +++ b/include/linux/blkdev.h
>>>> @@ -50,6 +50,9 @@ struct blk_stat_callback;
>>>>    /* Must be consistent with blk_mq_poll_stats_bkt() */
>>>>    #define BLK_MQ_POLL_STATS_BKTS 16
>>>>    +/* Using default classic polling */
>>>> +#define BLK_MQ_POLL_HYBRID_INVALID -1
> May be simply renaming this as BLK_MQ_POLL_CLASSIC would be even simpler ?
> That matches the comment and also the actual use of the macro, i.e.:
>
> if (q->poll_nsec == -1)
>
> means "do classic polling", which would nicely fits with:
>
> if (q->poll_nsec == BLK_MQ_POLL_CLASSIC)
>
> A lot easier to understand in my opinion. Using BLK_MQ_POLL_HYBRID_INVALID as a
> name is confusing with potential error cases where we actually want to do hybrid
> polling but the poll sleep time is invalid.

I think your suggestion is good. Thanks a lot.

Yufen
thanks

>>>> +
>>>>    /*
>>>>     * Maximum number of blkcg policies allowed to be registered
>>>> concurrently.
>>>>     * Defined here to simplify include dependency.
>>>>
>>> .
>>>
>>
>>
>