答复: [PATCH] page_pool: fill page only when refill condition is true

["Li,Rongqing" <lirongqing@baidu.com>]

> -----邮件原件-----
> 发件人: Jesper Dangaard Brouer [mailto:brouer@redhat.com]
> 发送时间: 2020年2月5日 19:51
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.kernel.org; brouer@redhat.com; Ilias Apalodimas
> <ilias.apalodimas@linaro.org>
> 主题: Re: [PATCH] page_pool: fill page only when refill condition is true
> 
> On Wed,  5 Feb 2020 16:22:34 +0800
> Li RongQing <lirongqing@baidu.com> wrote:
> 
> > "do {} while" in page_pool_refill_alloc_cache will always refill page
> > once whether refill is true or false, and whether alloc.count of pool
> > is less than PP_ALLOC_CACHE_REFILL.
> >
> > so fix it by calling page_pool_refill_alloc_cache() only when refill
> > is true
> >
> > Fixes: 44768decb7c0 ("page_pool: handle page recycle for NUMA_NO_NODE
> > condition")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> Hmmm... I'm not 100% convinced this is the right approach.
> 
> I do realize that in commit 44768decb7c0, I also added touching
> pool->alloc.cache[] which was protected by "called under" in_serving_softirq().
> (before I used a locked ptr_ring_consume(r)).
> 
> BUT maybe it will be better to remove, the test in_serving_softirq(), because
> the caller should provide guarantee that pool->alloc.cache[] is safe to access.
> 
> I added this in_serving_softirq() check, because I noticed NIC drivers will call
> this from normal process context, during (1) initial fill of their RX-rings, and (2)
> during driver RX-ring shutdown.  BUT in both cases the NIC drivers will first
> have made sure that their RX-ring have been disconnected and no concurrent
> accesses will happen.  Thus, access to pool->alloc.cache[] is safe, so
> page_pool API should trust the caller knows this.
> 
> 

I think it is better and semantic clarity to keep in_serving_softirq, and we can see opinions of others

Thanks

-Li