On 2021/7/21 22:06, Alexander Duyck wrote:
On Wed, Jul 21, 2021 at 1:15 AM Yunsheng Lin linyunsheng@huawei.com wrote:
On 2021/7/20 23:43, Alexander Duyck wrote:
On Mon, Jul 19, 2021 at 8:36 PM Yunsheng Lin linyunsheng@huawei.com wrote:
For 32 bit systems with 64 bit dma, dma_addr[1] is used to store the upper 32 bit dma addr, those system should be rare those days.
For normal system, the dma_addr[1] in 'struct page' is not used, so we can reuse dma_addr[1] for storing frag count, which means how many frags this page might be splited to.
In order to simplify the page frag support in the page pool, the PAGE_POOL_DMA_USE_PP_FRAG_COUNT macro is added to indicate the 32 bit systems with 64 bit dma, and the page frag support in page pool is disabled for such system.
The newly added page_pool_set_frag_count() is called to reserve the maximum frag count before any page frag is passed to the user. The page_pool_atomic_sub_frag_count_return() is called when user is done with the page frag.
Signed-off-by: Yunsheng Lin linyunsheng@huawei.com
include/linux/mm_types.h | 18 +++++++++++++----- include/net/page_pool.h | 41 ++++++++++++++++++++++++++++++++++------- net/core/page_pool.c | 4 ++++ 3 files changed, 51 insertions(+), 12 deletions(-)
<snip>
+static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
long nr)+{
long frag_count = atomic_long_read(&page->pp_frag_count);long ret;if (frag_count == nr)return 0;ret = atomic_long_sub_return(nr, &page->pp_frag_count);WARN_ON(ret < 0);return ret;}
So this should just be an atomic_long_sub_return call. You should get rid of the atomic_long_read portion of this as it can cover up reference count errors.
atomic_long_sub_return() is used to avoid one possible cache bouncing and barrrier caused by the last user.
I assume you mean "atomic_long_read()" here.
Yes, sorry for the confusion.
You are right that that may cover up the reference count errors. How about something like below:
static inline long page_pool_atomic_sub_frag_count_return(struct page *page, long nr) { #ifdef CONFIG_DEBUG_PAGE_REF long ret = atomic_long_sub_return(nr, &page->pp_frag_count);
WARN_ON(ret < 0); return ret;#else if (atomic_long_read(&page->pp_frag_count) == nr) return 0;
return atomic_long_sub_return(nr, &page->pp_frag_count);#end }
Or any better suggestion?
So the one thing I might change would be to make it so that you only do the atomic_long_read if nr is a constant via __builtin_constant_p. That way you would be performing the comparison in __page_pool_put_page and in the cases of freeing or draining the page_frags you would be using the atomic_long_sub_return which should be paths where you would not expect it to match or that are slowpath anyway.
Also I would keep the WARN_ON in both paths just to be on the safe side.
If I understand it correctly, we should change it as below, right?
static inline long page_pool_atomic_sub_frag_count_return(struct page *page, long nr) { long ret;
/* As suggested by Alexander, atomic_long_read() may cover up the * reference count errors, so avoid calling atomic_long_read() in * the cases of freeing or draining the page_frags, where we would * not expect it to match or that are slowpath anyway. */ if (__builtin_constant_p(nr) && atomic_long_read(&page->pp_frag_count) == nr) return 0;
ret = atomic_long_sub_return(nr, &page->pp_frag_count); WARN_ON(ret < 0); return ret; }
.