Ming Lei <[email protected]> writes:
> You can add
> Reviewed-by: Ming Lei <[email protected]>
> if the following trivial issues(especially the 2nd one) are addressed.
[snip]
>> @@ -1891,7 +1890,12 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q)
>>
>> mutex_lock(&set->tag_list_lock);
>> list_del_init(&q->tag_set_list);
>> - blk_mq_update_tag_set_depth(set);
>> + if (set->tag_list.next == set->tag_list.prev) {
>
> list_is_singular() should be better.
Didn't even know that existed. Thanks.
>> + /* just transitioned to unshared */
>> + set->flags &= ~BLK_MQ_F_TAG_SHARED;
>> + /* update existing queue */
>> + blk_mq_update_tag_set_depth(set, false);
>> + }
>> mutex_unlock(&set->tag_list_lock);
>> }
>>
>> @@ -1901,8 +1905,17 @@ static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set,
>> q->tag_set = set;
>>
>> mutex_lock(&set->tag_list_lock);
>> +
>> + /* Check to see if we're transitioning to shared (from 1 to 2 queues). */
>> + if (!list_empty(&set->tag_list) && !(set->flags & BLK_MQ_F_TAG_SHARED)) {
>> + set->flags |= BLK_MQ_F_TAG_SHARED;
>> + /* update existing queue */
>> + blk_mq_update_tag_set_depth(set, true);
>> + }
>> + if (set->flags & BLK_MQ_F_TAG_SHARED)
>
> The above should be 'else if', otherwise the current queue will be set
> twice.
I moved the list add below this to avoid that very issue. See:
>> + queue_set_hctx_shared(q, true);
>> list_add_tail(&q->tag_set_list, &set->tag_list);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This seemed the cleanest way to structure the code to avoid the double
walking of the hctx list for the current q.
-Jeff
>> - blk_mq_update_tag_set_depth(set);
>> +
>> mutex_unlock(&set->tag_list_lock);
>> }
>>
On Mon, Nov 2, 2015 at 10:04 PM, Jeff Moyer <[email protected]> wrote:
> Ming Lei <[email protected]> writes:
>
>> You can add
>> Reviewed-by: Ming Lei <[email protected]>
>> if the following trivial issues(especially the 2nd one) are addressed.
>
> [snip]
>
>>> @@ -1891,7 +1890,12 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q)
>>>
>>> mutex_lock(&set->tag_list_lock);
>>> list_del_init(&q->tag_set_list);
>>> - blk_mq_update_tag_set_depth(set);
>>> + if (set->tag_list.next == set->tag_list.prev) {
>>
>> list_is_singular() should be better.
>
> Didn't even know that existed. Thanks.
>
>>> + /* just transitioned to unshared */
>>> + set->flags &= ~BLK_MQ_F_TAG_SHARED;
>>> + /* update existing queue */
>>> + blk_mq_update_tag_set_depth(set, false);
>>> + }
>>> mutex_unlock(&set->tag_list_lock);
>>> }
>>>
>>> @@ -1901,8 +1905,17 @@ static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set,
>>> q->tag_set = set;
>>>
>>> mutex_lock(&set->tag_list_lock);
>>> +
>>> + /* Check to see if we're transitioning to shared (from 1 to 2 queues). */
>>> + if (!list_empty(&set->tag_list) && !(set->flags & BLK_MQ_F_TAG_SHARED)) {
>>> + set->flags |= BLK_MQ_F_TAG_SHARED;
>>> + /* update existing queue */
>>> + blk_mq_update_tag_set_depth(set, true);
>>> + }
>>> + if (set->flags & BLK_MQ_F_TAG_SHARED)
>>
>> The above should be 'else if', otherwise the current queue will be set
>> twice.
>
> I moved the list add below this to avoid that very issue. See:
>
>>> + queue_set_hctx_shared(q, true);
>>> list_add_tail(&q->tag_set_list, &set->tag_list);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This seemed the cleanest way to structure the code to avoid the double
> walking of the hctx list for the current q.
OK, it is correct, then v1 is fine.
Reviewed-by: Ming Lei <[email protected]>
>
> -Jeff
>
>>> - blk_mq_update_tag_set_depth(set);
>>> +
>>> mutex_unlock(&set->tag_list_lock);
>>> }
>>>
Ming Lei <[email protected]> writes:
>>> The above should be 'else if', otherwise the current queue will be set
>>> twice.
>>
>> I moved the list add below this to avoid that very issue. See:
>>
>>>> + queue_set_hctx_shared(q, true);
>>>> list_add_tail(&q->tag_set_list, &set->tag_list);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> This seemed the cleanest way to structure the code to avoid the double
>> walking of the hctx list for the current q.
>
> OK, it is correct, then v1 is fine.
>
> Reviewed-by: Ming Lei <[email protected]>
Thanks, Ming. Jens, I'll re-send with the list_is_singular change and
this one should be ready for merging.
Cheers,
Jeff
On 11/03/2015 06:27 AM, Jeff Moyer wrote:
> Ming Lei <[email protected]> writes:
>
>>>> The above should be 'else if', otherwise the current queue will be set
>>>> twice.
>>>
>>> I moved the list add below this to avoid that very issue. See:
>>>
>>>>> + queue_set_hctx_shared(q, true);
>>>>> list_add_tail(&q->tag_set_list, &set->tag_list);
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> This seemed the cleanest way to structure the code to avoid the double
>>> walking of the hctx list for the current q.
>>
>> OK, it is correct, then v1 is fine.
>>
>> Reviewed-by: Ming Lei <[email protected]>
>
> Thanks, Ming. Jens, I'll re-send with the list_is_singular change and
> this one should be ready for merging.
Great, thanks Jeff!
--
Jens Axboe