Re: [PATCH RFC net-next/mm V3 1/2] page_pool: Remove workqueue in new shutdown scheme

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2023/5/4 21:48, Jesper Dangaard Brouer wrote:
> On 04/05/2023 04.42, Yunsheng Lin wrote:
>> On 2023/4/29 0:16, Jesper Dangaard Brouer wrote:
>>>   void page_pool_release_page(struct page_pool *pool, struct page *page)
>>>   {
>>> +    unsigned int flags = READ_ONCE(pool->p.flags);
>>>       dma_addr_t dma;
>>> -    int count;
>>> +    u32 release_cnt;
>>> +    u32 hold_cnt;
>>>         if (!(pool->p.flags & PP_FLAG_DMA_MAP))
>>>           /* Always account for inflight pages, even if we didn't
>>> @@ -490,11 +503,15 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
>>>   skip_dma_unmap:
>>>       page_pool_clear_pp_info(page);
>>>   -    /* This may be the last page returned, releasing the pool, so
>>> -     * it is not safe to reference pool afterwards.
>>> -     */
>>> -    count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
>>> -    trace_page_pool_state_release(pool, page, count);
>>
>> There is a time window between "unsigned int flags = READ_ONCE(pool->p.flags)"
>> and flags checking, if page_pool_destroy() is called concurrently during that
>> time window, it seems we will have a pp instance leaking problem here?
>>
> 
> Nope, that is resolved by the code changes in page_pool_destroy(), see below.

Maybe I did not describe the data race clearly enough.

         CPU 0                                               CPU1
	   .
	   .
unsigned int flags = READ_ONCE(pool->p.flags);
	   .
	   .                                         page_pool_destroy()
	   .
atomic_inc_return(&pool->pages_state_release_cnt)
           .
	   .
	   .
 if (flags & PP_FLAG_SHUTDOWN)
	page_pool_free_attempt();

The above data race may cause a pp instance leaking problem:
CPU0 is releasing the last page for a pp and it did not see the pool->p.flags
with the PP_FLAG_SHUTDOWN set because page_pool_destroy() is called after
reading pool->p.flags, so page_pool_free_attempt() is not called to free
pp.

CPU1 calling the page_pool_destroy() also did not free pp as CPU0 had not
done the atomic_inc_return() for pool->pages_state_release_cnt yet.

Or did I miss something obvious here?

> 
>> It seems it is very hard to aovid this kind of corner case when using both
>> flags & PP_FLAG_SHUTDOWN and release_cnt/hold_cnt checking to decide if pp
>> instance can be freed.
>> Can we use something like biased reference counting, which used by frag support
>> in page pool? So that we only need to check only one variable and avoid cache
>> bouncing as much as possible.
>>
> 
> See below, I believe we are doing an equivalent refcnt bias trick, that
> solves these corner cases in page_pool_destroy().
> In short: hold_cnt is increased, prior to setting PP_FLAG_SHUTDOWN.
> Thus, if this code READ_ONCE flags without PP_FLAG_SHUTDOWN, we know it
> will not be the last to release pool->pages_state_release_cnt.

It is not exactly the kind of refcnt bias trick in my mind, I was thinking
about using pool->pages_state_hold_cnt as refcnt bias and merge it to
pool->pages_state_release_cnt as needed, maybe I need to try to implement
that to see if it turn out to be what I want it to be.

> Below: Perhaps, we should add a RCU grace period to make absolutely
> sure, that this code completes before page_pool_destroy() call completes.
> 
> 
>>> +    if (flags & PP_FLAG_SHUTDOWN)
>>> +        hold_cnt = pp_read_hold_cnt(pool);
>>> +
> 
> I would like to avoid above code, and I'm considering using call_rcu(),
> which I think will resolve the race[0] this code deals with.
> As I explained here[0], this code deals with another kind of race.

Yes, I understand that. I even went to check if the below tracepoint
trace_page_pool_state_release() was causing a use-after-free problem
as it is passing 'pool':)

> 
>  [0] https://lore.kernel.org/all/f671f5da-d9bc-a559-2120-10c3491e6f6d@redhat.com/
> 
>>> +    release_cnt = atomic_inc_return(&pool->pages_state_release_cnt);
>>> +    trace_page_pool_state_release(pool, page, release_cnt);
>>> +
>>> +    /* In shutdown phase, last page will free pool instance */
>>> +    if (flags & PP_FLAG_SHUTDOWN)
>>> +        page_pool_free_attempt(pool, hold_cnt, release_cnt);
>>>   }
>>>   EXPORT_SYMBOL(page_pool_release_page);
>>>
>>
>> ...
>>
>>>     void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
>>> @@ -856,6 +884,10 @@ EXPORT_SYMBOL(page_pool_unlink_napi);
>>>     void page_pool_destroy(struct page_pool *pool)
>>>   {
>>> +    unsigned int flags;
>>> +    u32 release_cnt;
>>> +    u32 hold_cnt;
>>> +
>>>       if (!pool)
>>>           return;
>>>   @@ -868,11 +900,39 @@ void page_pool_destroy(struct page_pool *pool)
>>>       if (!page_pool_release(pool))
>>>           return;
>>>   -    pool->defer_start = jiffies;
>>> -    pool->defer_warn  = jiffies + DEFER_WARN_INTERVAL;
>>> +    /* PP have pages inflight, thus cannot immediately release memory.
>>> +     * Enter into shutdown phase, depending on remaining in-flight PP
>>> +     * pages to trigger shutdown process (on concurrent CPUs) and last
>>> +     * page will free pool instance.
>>> +     *
>>> +     * There exist two race conditions here, we need to take into
>>> +     * account in the following code.
>>> +     *
>>> +     * 1. Before setting PP_FLAG_SHUTDOWN another CPU released the last
>>> +     *    pages into the ptr_ring.  Thus, it missed triggering shutdown
>>> +     *    process, which can then be stalled forever.
>>> +     *
>>> +     * 2. After setting PP_FLAG_SHUTDOWN another CPU released the last
>>> +     *    page, which triggered shutdown process and freed pool
>>> +     *    instance. Thus, its not safe to dereference *pool afterwards.
>>> +     *
>>> +     * Handling races by holding a fake in-flight count, via
>>> +     * artificially bumping pages_state_hold_cnt, which assures pool
>>> +     * isn't freed under us.  For race(1) its safe to recheck ptr_ring
>>> +     * (it will not free pool). Race(2) cannot happen, and we can
>>> +     * release fake in-flight count as last step.
>>> +     */
>>> +    hold_cnt = READ_ONCE(pool->pages_state_hold_cnt) + 1;
>>> +    smp_store_release(&pool->pages_state_hold_cnt, hold_cnt);
>>
>> I assume the smp_store_release() is used to ensure the correct order
>> between the above store operations?
>> There is data dependency between those two store operations, do we
>> really need the smp_store_release() here?
>>
>>> +    barrier();
>>> +    flags = READ_ONCE(pool->p.flags) | PP_FLAG_SHUTDOWN;
>>
>> Do we need a stronger barrier like smp_rmb() to prevent cpu from
>> executing "flags = READ_ONCE(pool->p.flags) | PP_FLAG_SHUTDOWN"
>> before "smp_store_release(&pool->pages_state_hold_cnt, hold_cnt)"
>> even if there is a smp_store_release() barrier here?
>>
> I do see you point and how it is related to your above comment for
> page_pool_release_page().
> 
> I think we need to replace barrier() with synchronize_rcu().
> Meaning we add a RCU grace period to "wait" for above code (in
> page_pool_release_page) that read the old flags value to complete.
> 
> 
>>> +    smp_store_release(&pool->p.flags, flags);
> 
> When doing a synchronize_rcu(), I assume this smp_store_release() is
> overkill, right?
> Will a WRITE_ONCE() be sufficient?
> 
> Hmm, the synchronize_rcu(), shouldn't that be *after* storing the flags?

Yes.
As my understanding, we probably do not need any of those *_ONCE() and
barrier when using rcu.

But I am not really convinced that we need to go for rcu yet.