From: John Fastabend <john.fastabend@gmail.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: jasowang@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] net: use cmpxchg instead of spinlock in ptr rings
Date: Tue, 15 Nov 2016 20:30:09 -0800 [thread overview]
Message-ID: <582BE0D1.3020007@gmail.com> (raw)
In-Reply-To: <20161115002552-mutt-send-email-mst@kernel.org>
On 16-11-14 03:01 PM, Michael S. Tsirkin wrote:
> On Thu, Nov 10, 2016 at 08:44:08PM -0800, John Fastabend wrote:
>>
>> ---
>> include/linux/ptr_ring_ll.h | 136 +++++++++++++++++++++++++++++++++++++++++++
>> include/linux/skb_array.h | 25 ++++++++
>> 2 files changed, 161 insertions(+)
>> create mode 100644 include/linux/ptr_ring_ll.h
>>
>> diff --git a/include/linux/ptr_ring_ll.h b/include/linux/ptr_ring_ll.h
>> new file mode 100644
>> index 0000000..bcb11f3
>> --- /dev/null
>> +++ b/include/linux/ptr_ring_ll.h
>> @@ -0,0 +1,136 @@
>> +/*
>> + * Definitions for the 'struct ptr_ring_ll' datastructure.
>> + *
>> + * Author:
>> + * John Fastabend <john.r.fastabend@intel.com>
>> + *
>> + * Copyright (C) 2016 Intel Corp.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + *
>> + * This is a limited-size FIFO maintaining pointers in FIFO order, with
>> + * one CPU producing entries and another consuming entries from a FIFO.
>> + * extended from ptr_ring_ll to use cmpxchg over spin lock.
>
> So when is each one (ptr-ring/ptr-ring-ll) a win? _ll suffix seems to
> imply this gives a better latency, OTOH for a ping/pong I suspect
> ptr-ring would be better as it avoids index cache line bounces.
My observation under qdisc testing with pktgen is that I get better pps
numbers with this code vs ptr_ring using spinlock. I actually wrote
this implementation before the skb_array code was around though and
haven't done a thorough analysis of the two yet only pktgen benchmarks.
In my pktgen benchmarks I test 1:1 producer/consumer and many to one
producer/consumer tests. I'll post some numbers later this week.
[...]
>> + */
>> +static inline int __ptr_ring_ll_produce(struct ptr_ring_ll *r, void *ptr)
>> +{
>> + u32 ret, head, tail, next, slots, mask;
>> +
>> + do {
>> + head = READ_ONCE(r->prod_head);
>> + mask = READ_ONCE(r->prod_mask);
>> + tail = READ_ONCE(r->cons_tail);
>> +
>> + slots = mask + tail - head;
>> + if (slots < 1)
>> + return -ENOMEM;
>> +
>> + next = head + 1;
>> + ret = cmpxchg(&r->prod_head, head, next);
>> + } while (ret != head);
>
>
> So why is this preferable to a lock?
>
> I suspect it's nothing else than the qspinlock fairness
> and polling code complexity. It's all not very useful if you
> 1. are just doing a couple of instructions under the lock
> and
> 2. use a finite FIFO which is unfair anyway
>
>
> How about this hack (lifted from virt_spin_lock):
>
> static inline void quick_spin_lock(struct qspinlock *lock)
> {
> do {
> while (atomic_read(&lock->val) != 0)
> cpu_relax();
> } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> }
>
> Or maybe we should even drop the atomic_read in the middle -
> worth profiling and comparing:
>
> static inline void quick_spin_lock(struct qspinlock *lock)
> {
> while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
> cpu_relax();
> }
>
>
> Then, use quick_spin_lock instead of spin_lock everywhere in
> ptr_ring - will that make it more efficient?
>
I think this could be the case. I'll give it a test later this week I
am working on the xdp bits for virtio at the moment. To be honest though
for my qdisc patchset first I need to resolve a bug and then probably in
the first set just use the existing skb_array implementation. Its fun
to micro-optimize this stuff but really any implementation will show
improvement over existing code.
Thanks,
John
next prev parent reply other threads:[~2016-11-16 4:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-11 4:43 [RFC PATCH 0/2] illustrate cmpxchg ring for tap/tun and qdisc John Fastabend
2016-11-11 4:44 ` [RFC PATCH 1/2] net: use cmpxchg instead of spinlock in ptr rings John Fastabend
2016-11-14 11:09 ` Jesper Dangaard Brouer
2016-11-14 23:01 ` Michael S. Tsirkin
2016-11-16 4:30 ` John Fastabend [this message]
2016-11-11 4:44 ` [RFC PATCH 2/2] ptr_ring_ll: pop/push multiple objects at once John Fastabend
2016-11-14 23:06 ` Michael S. Tsirkin
2016-11-16 4:42 ` John Fastabend
2016-11-16 5:23 ` 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=582BE0D1.3020007@gmail.com \
--to=john.fastabend@gmail.com \
--cc=jasowang@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.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).