Date:   Fri, 28 May 2021 10:29:27 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Yunsheng Lin <linyunsheng@...wei.com>, davem@...emloft.net,
        kuba@...nel.org
Cc:     will@...nel.org, peterz@...radead.org, paulmck@...nel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        mst@...hat.com, brouer@...hat.com
Subject: Re: [PATCH net-next] ptr_ring: make __ptr_ring_empty() checking more
 reliable


在 2021/5/28 上午10:26, Yunsheng Lin 写道:
> On 2021/5/28 9:31, Jason Wang wrote:
>> 在 2021/5/27 下午5:03, Yunsheng Lin 写道:
>>> On 2021/5/27 16:05, Jason Wang wrote:
>>>> 在 2021/5/27 下午3:21, Yunsheng Lin 写道:
>>>>> On 2021/5/27 14:53, Jason Wang wrote:
>>>>>> 在 2021/5/27 下午2:07, Yunsheng Lin 写道:
>>>>>>> On 2021/5/27 12:57, Jason Wang wrote:
>>>>>>>> 在 2021/5/26 下午8:29, Yunsheng Lin 写道:
>>>>>>>>> Currently r->queue[] is cleared after r->consumer_head is moved
>>>>>>>>> forward, which makes the __ptr_ring_empty() checking called in
>>>>>>>>> page_pool_refill_alloc_cache() unreliable if the checking is done
>>>>>>>>> after the r->queue clearing and before the consumer_head moving
>>>>>>>>> forward.
>>>>>>>>>
>>>>>>>>> Move the r->queue[] clearing after consumer_head moving forward
>>>>>>>>> to make __ptr_ring_empty() checking more reliable.
>>>>>>>> If I understand this correctly, this can only happens if you run __ptr_ring_empty() in parallel with ptr_ring_discard_one().
>>>>>>> Yes.
>>>>>>>
>>>>>>>> I think those two needs to be serialized. Or did I miss anything?
>>>>>>> As the below comment in __ptr_ring_discard_one, if the above is true, I
>>>>>>> do not think we need to keep consumer_head valid at all times, right?
>>>>>>>
>>>>>>>
>>>>>>>        /* Note: we must keep consumer_head valid at all times for __ptr_ring_empty
>>>>>>>         * to work correctly.
>>>>>>>         */
>>>>>> I'm not sure I understand. But my point is that you need to synchronize the __ptr_ring_discard_one() and __ptr_empty() as explained in the comment above __ptr_ring_empty():
>>>>> I am saying if __ptr_ring_empty() and __ptr_ring_discard_one() is
>>>>> always serialized, then it seems that the below commit is unnecessary?
>>>> Just to make sure we are at the same page. What I really meant is "synchronized" not "serialized". So they can be called at the same time but need synchronization.
>>>>
>>>>
>>>>> 406de7555424 ("ptr_ring: keep consumer_head valid at all times")
>>>> This still needed in this case.
>>>>
>>>>
>>>>>> /*
>>>>>>     * Test ring empty status without taking any locks.
>>>>>>     *
>>>>>>     * NB: This is only safe to call if ring is never resized.
>>>>>>     *
>>>>>>     * However, if some other CPU consumes ring entries at the same time, the value
>>>>>>     * returned is not guaranteed to be correct.
>>>>>>     *
>>>>>>     * In this case - to avoid incorrectly detecting the ring
>>>>>>     * as empty - the CPU consuming the ring entries is responsible
>>>>>>     * for either consuming all ring entries until the ring is empty,
>>>>>>     * or synchronizing with some other CPU and causing it to
>>>>>>     * re-test __ptr_ring_empty and/or consume the ring enteries
>>>>>>     * after the synchronization point.
>>>>> I am not sure I understand "incorrectly detecting the ring as empty"
>>>>> means, is it because of the data race described in the commit log?
>>>> It means "the ring might be empty but __ptr_ring_empty() returns false".
>>> But the ring might be non-empty but __ptr_ring_empty() returns true
>>> for the data race described in the commit log:)
>>
>> Which commit log?
> this commit log.
> If the data race described in this commit log happens, the ring might be
> non-empty, but __ptr_ring_empty() returns true.
>
>
>>
>>>>> Or other data race? I can not think of other data race if consuming
>>>>> and __ptr_ring_empty() is serialized:)
>>>>>
>>>>> I am agreed that __ptr_ring_empty() checking is not totally reliable
>>>>> without taking r->consumer_lock, that is why I use "more reliable"
>>>>> in the title:)
>>>> Is __ptr_ring_empty() synchronized with the consumer in your case? If yes, have you done some benchmark to see the difference?
>>>>
>>>> Have a look at page pool, this only helps when multiple refill request happens in parallel which can make some of the refill return early if the ring has been consumed.
>>>>
>>>> This is the slow-path and I'm not sure we see any difference. If one the request runs faster then the following request will go through the fast path.
>>> Yes, I am agreed there may not be any difference.
>>> But it is better to make it more reliable, right?
>>
>> No, any performance optimization must be benchmark to show obvious difference to be accepted.
>>
>> ptr_ring has been used by various subsystems so we should not risk our self-eves to accept theoretical optimizations.
> As a matter of fact, I am not treating it as a performance optimization for this patch.
> I treated it as improvement for the checking of __ptr_ring_empty().
> But you are right that we need to ensure there is not performance regression when improving
> it.
>
> Any existing and easy-to-setup testcase to benchmark the ptr_ring performance?


You probably can start with a simple test in:

tools/virtio/ringtest/ptr_ring.c

Thanks


>
>>
>>>> If it really helps, can we do it more simpler by:
>>>>
>

Powered by blists - more mailing lists