From: "Michael S. Tsirkin" <mst@redhat.com>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: davem@davemloft.net, kuba@kernel.org, jasowang@redhat.com,
brouer@redhat.com, paulmck@kernel.org, peterz@infradead.org,
will@kernel.org, shuah@kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, linux-kselftest@vger.kernel.org,
linuxarm@openeuler.org
Subject: Re: [PATCH net-next v2 2/2] ptr_ring: make __ptr_ring_empty() checking more reliable
Date: Fri, 25 Jun 2021 02:39:34 -0400 [thread overview]
Message-ID: <20210625023749-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <1624591136-6647-3-git-send-email-linyunsheng@huawei.com>
On Fri, Jun 25, 2021 at 11:18:56AM +0800, Yunsheng Lin wrote:
> 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.
>
> As a side effect of above change, a consumer_head checking is
> avoided for the likely case, and it has noticeable performance
> improvement when it is tested using the ptr_ring_test selftest
> added in the previous patch.
>
> Using "taskset -c 1 ./ptr_ring_test -s 1000 -m 0 -N 100000000"
> to test the case of single thread doing both the enqueuing and
> dequeuing:
>
> arch unpatched patched delta
> arm64 4648 ms 4464 ms +3.9%
> X86 2562 ms 2401 ms +6.2%
>
> Using "taskset -c 1-2 ./ptr_ring_test -s 1000 -m 1 -N 100000000"
> to test the case of one thread doing enqueuing and another thread
> doing dequeuing concurrently, also known as single-producer/single-
> consumer:
>
> arch unpatched patched delta
> arm64 3624 ms + 3624 ms 3462 ms + 3462 ms +4.4%
> x86 2758 ms + 2758 ms 2547 ms + 2547 ms +7.6%
Nice but it's small - could be a fluke.
How many tests did you run? What is the variance?
Did you try pinning to different CPUs to observe numa effects?
Please use perf or some other modern tool for this kind
of benchmark. Thanks!
>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
> V2: Add performance data.
> ---
> include/linux/ptr_ring.h | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 808f9d3..db9c282 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -261,8 +261,7 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
> /* Note: we must keep consumer_head valid at all times for __ptr_ring_empty
> * to work correctly.
> */
> - int consumer_head = r->consumer_head;
> - int head = consumer_head++;
> + int consumer_head = r->consumer_head + 1;
>
> /* Once we have processed enough entries invalidate them in
> * the ring all at once so producer can reuse their space in the ring.
> @@ -271,19 +270,27 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
> */
> if (unlikely(consumer_head - r->consumer_tail >= r->batch ||
> consumer_head >= r->size)) {
> + int tail = r->consumer_tail;
> +
> + if (unlikely(consumer_head >= r->size)) {
> + r->consumer_tail = 0;
> + WRITE_ONCE(r->consumer_head, 0);
> + } else {
> + r->consumer_tail = consumer_head;
> + WRITE_ONCE(r->consumer_head, consumer_head);
> + }
> +
> /* Zero out entries in the reverse order: this way we touch the
> * cache line that producer might currently be reading the last;
> * producer won't make progress and touch other cache lines
> * besides the first one until we write out all entries.
> */
> - while (likely(head >= r->consumer_tail))
> - r->queue[head--] = NULL;
> - r->consumer_tail = consumer_head;
> - }
> - if (unlikely(consumer_head >= r->size)) {
> - consumer_head = 0;
> - r->consumer_tail = 0;
> + while (likely(--consumer_head >= tail))
> + r->queue[consumer_head] = NULL;
> +
> + return;
> }
> +
> /* matching READ_ONCE in __ptr_ring_empty for lockless tests */
> WRITE_ONCE(r->consumer_head, consumer_head);
> }
> --
> 2.7.4
next prev parent reply other threads:[~2021-06-25 6:40 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-25 3:18 [PATCH net-next v2 0/2] add benchmark selftest and optimization for ptr_ring Yunsheng Lin
2021-06-25 3:18 ` [PATCH net-next v2 1/2] selftests/ptr_ring: add benchmark application " Yunsheng Lin
2021-06-25 3:36 ` Jason Wang
2021-06-25 3:52 ` Yunsheng Lin
2021-06-27 6:09 ` Michael S. Tsirkin
2021-06-28 1:42 ` Yunsheng Lin
2021-06-25 6:37 ` Michael S. Tsirkin
2021-06-25 7:40 ` Yunsheng Lin
2021-06-25 3:18 ` [PATCH net-next v2 2/2] ptr_ring: make __ptr_ring_empty() checking more reliable Yunsheng Lin
2021-06-25 6:32 ` Michael S. Tsirkin
2021-06-25 7:21 ` Yunsheng Lin
2021-06-25 7:30 ` Michael S. Tsirkin
2021-06-25 8:33 ` Yunsheng Lin
2021-06-27 6:03 ` Michael S. Tsirkin
2021-06-28 2:17 ` Yunsheng Lin
2021-06-25 6:39 ` Michael S. Tsirkin [this message]
2021-06-25 9:20 ` Yunsheng Lin
2021-06-27 6:07 ` Michael S. Tsirkin
2021-06-28 2:11 ` [Linuxarm] " Yunsheng Lin
2021-06-25 6:42 ` [PATCH net-next v2 0/2] add benchmark selftest and optimization for ptr_ring Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210625023749-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=brouer@redhat.com \
--cc=davem@davemloft.net \
--cc=jasowang@redhat.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linuxarm@openeuler.org \
--cc=linyunsheng@huawei.com \
--cc=netdev@vger.kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=shuah@kernel.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).