From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-06-02 16:11:12
Note: Dave, I agree with your comment this infrastructure should only be
merged together with Jason's patches using it. Jason, I tagged them RFC
accordingly - please repost them together with the tun patch. Posting
here to make it easier for Jason to pick it up and use.
This is in response to the proposal by Jason to make tun
rx packet queue lockless using a circular buffer.
My testing seems to show that at least for the common usecase
in networking, which isn't lockless, circular buffer
with indices does not perform that well, because
each index access causes a cache line to bounce between
CPUs, and index access causes stalls due to the dependency.
By comparison, an array of pointers where NULL means invalid
and !NULL means valid, can be updated without messing up barriers
at all and does not have this issue.
On the flip side, cache pressure may be caused by using large queues.
tun has a queue of 1000 entries by default and that's 8K.
At this point I'm not sure this can be solved efficiently.
The correct solution might be sizing the queues appropriately.
Here's an implementation of this idea: it can be used more
or less whenever sk_buff_head can be used, except you need
to know the queue size in advance.
As this might be useful outside of networking, I implemented
a generic array of void pointers, with a type-safe wrapper for skbs.
It remains to be seen whether resizing is required, in case it is
I included patches implementing resizing by holding both the
consumer and the producer locks.
I think this code works fine without any extra memory barriers since we
always read and write the same location, so the accesses can not be
reordered.
Multiple writes of the same value into memory would mess things up
for us, I don't think compilers would do it though.
But if people feel it's better to be safe wrt compiler optimizations,
specifying queue as volatile would probably do it in a cleaner way
than converting all accesses to READ_ONCE/WRITE_ONCE. Thoughts?
The only issue is with calls within a loop using the __ptr_ring_XXX
accessors - in theory compiler could hoist accesses out of the loop.
Following volatile-considered-harmful.txt I merely
documented that callers that busy-poll should invoke cpu_relax().
Most people will use the external skb_array_XXX APIs with a spinlock,
so this should not be an issue for them.
Eric Dumazet suggested adding an extra pointer to skb for when
we have a single outstanding packet. I could not figure out
a way to implement this without a shared consumer/producer lock
though, which would cause cache line bounces by itself.
changes since v6
resize implemented. peek/full calls are no longer lockless
replaced _FIELD macros with _CALL which invoke a function
on the pointer rather than just returning a value
destroy now scans the array and frees all queued skbs
changes since v5
implemented a generic ptr_ring api, and
made skb_array a type-safe wrapper
apis for taking the spinlock in different contexts
following expected usecase in tun
changes since v4 (v3 was never posted)
documentation
dropped SKB_ARRAY_MIN_SIZE heuristic
unit test (in userspace, included as patch 2)
changes since v2:
fixed integer overflow pointed out by Eric.
added some comments.
changes since v1:
fixed bug pointed out by Eric.
** BLURB HERE ***
Michael S. Tsirkin (5):
ptr_ring: array based FIFO for pointers
ptr_ring: ring test
skb_array: array based FIFO for skbs
ptr_ring: resize support
skb_array: resize support
include/linux/ptr_ring.h | 393 +++++++++++++++++++++++++++++++++++++++
include/linux/skb_array.h | 168 +++++++++++++++++
tools/virtio/ringtest/ptr_ring.c | 192 +++++++++++++++++++
tools/virtio/ringtest/Makefile | 5 +-
4 files changed, 757 insertions(+), 1 deletion(-)
create mode 100644 include/linux/ptr_ring.h
create mode 100644 include/linux/skb_array.h
create mode 100644 tools/virtio/ringtest/ptr_ring.c
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-06-02 16:08:25
A simple array based FIFO of pointers. Intended for net stack which
commonly has a single consumer/producer.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/ptr_ring.h | 264 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 264 insertions(+)
create mode 100644 include/linux/ptr_ring.h
@@ -0,0 +1,264 @@+/*+*Definitionsforthe'structptr_ring'datastructure.+*+*Author:+*MichaelS.Tsirkin<mst@redhat.com>+*+*Copyright(C)2016RedHat,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsoftheGNUGeneralPublicLicenseaspublishedbythe+*FreeSoftwareFoundation;eitherversion2oftheLicense,or(atyour+*option)anylaterversion.+*+*Thisisalimited-sizeFIFOmaintainingpointersinFIFOorder,with+*oneCPUproducingentriesandanotherconsumingentriesfromaFIFO.+*+*Thisimplementationtriestominimizecache-contentionwhenthereisa+*singleproducerandasingleconsumerCPU.+*/++#ifndef _LINUX_PTR_RING_H+#define _LINUX_PTR_RING_H 1++#ifdef __KERNEL__+#include<linux/spinlock.h>+#include<linux/cache.h>+#include<linux/types.h>+#include<linux/compiler.h>+#include<linux/cache.h>+#include<linux/slab.h>+#include<asm/errno.h>+#endif++structptr_ring{+intproducer____cacheline_aligned_in_smp;+spinlock_tproducer_lock;+intconsumer____cacheline_aligned_in_smp;+spinlock_tconsumer_lock;+/* Shared consumer/producer data */+/* Read-only by both the producer and the consumer */+intsize____cacheline_aligned_in_smp;/* max entries in queue */+void**queue;+};++/* Note: callers invoking this in a loop must use a compiler barrier,+*forexamplecpu_relax().+*Callersdon'tneedtotakeproducerlock-iftheydon't+*thenextcallto__ptr_ring_producemayfail.+*/+staticinlinebool__ptr_ring_full(structptr_ring*r)+{+returnr->queue[r->producer];+}++staticinlineboolptr_ring_full(structptr_ring*r)+{+barrier();+return__ptr_ring_full(r);+}++/* Note: callers invoking this in a loop must use a compiler barrier,+*forexamplecpu_relax().+*/+staticinlineint__ptr_ring_produce(structptr_ring*r,void*ptr)+{+if(__ptr_ring_full(r))+return-ENOSPC;++r->queue[r->producer++]=ptr;+if(unlikely(r->producer>=r->size))+r->producer=0;+return0;+}++staticinlineintptr_ring_produce(structptr_ring*r,void*ptr)+{+intret;++spin_lock(&r->producer_lock);+ret=__ptr_ring_produce(r,ptr);+spin_unlock(&r->producer_lock);++returnret;+}++staticinlineintptr_ring_produce_irq(structptr_ring*r,void*ptr)+{+intret;++spin_lock_irq(&r->producer_lock);+ret=__ptr_ring_produce(r,ptr);+spin_unlock_irq(&r->producer_lock);++returnret;+}++staticinlineintptr_ring_produce_any(structptr_ring*r,void*ptr)+{+unsignedlongflags;+intret;++spin_lock_irqsave(&r->producer_lock,flags);+ret=__ptr_ring_produce(r,ptr);+spin_unlock_irqrestore(&r->producer_lock,flags);++returnret;+}++staticinlineintptr_ring_produce_bh(structptr_ring*r,void*ptr)+{+intret;++spin_lock_bh(&r->producer_lock);+ret=__ptr_ring_produce(r,ptr);+spin_unlock_bh(&r->producer_lock);++returnret;+}++/* Note: callers invoking this in a loop must use a compiler barrier,+*forexamplecpu_relax().Callersmusttakeconsumer_lock+*iftheydereferencethepointer-seee.g.PTR_RING_PEEK_CALL.+*There'snoneedforalockifpointerismerelytested-seee.g.+*ptr_ring_empty.+*/+staticinlinevoid*__ptr_ring_peek(structptr_ring*r)+{+returnr->queue[r->consumer];+}++staticinlineboolptr_ring_empty(structptr_ring*r)+{+barrier();+return!__ptr_ring_peek(r);+}++/* Must only be called after __ptr_ring_peek returned !NULL */+staticinlinevoid__ptr_ring_discard_one(structptr_ring*r)+{+r->queue[r->consumer++]=NULL;+if(unlikely(r->consumer>=r->size))+r->consumer=0;+}++staticinlinevoid*__ptr_ring_consume(structptr_ring*r)+{+void*ptr;++ptr=__ptr_ring_peek(r);+if(ptr)+__ptr_ring_discard_one(r);++returnptr;+}++staticinlinevoid*ptr_ring_consume(structptr_ring*r)+{+void*ptr;++spin_lock(&r->consumer_lock);+ptr=__ptr_ring_consume(r);+spin_unlock(&r->consumer_lock);++returnptr;+}++staticinlinevoid*ptr_ring_consume_irq(structptr_ring*r)+{+void*ptr;++spin_lock_irq(&r->consumer_lock);+ptr=__ptr_ring_consume(r);+spin_unlock_irq(&r->consumer_lock);++returnptr;+}++staticinlinevoid*ptr_ring_consume_any(structptr_ring*r)+{+unsignedlongflags;+void*ptr;++spin_lock_irqsave(&r->consumer_lock,flags);+ptr=__ptr_ring_consume(r);+spin_unlock_irqrestore(&r->consumer_lock,flags);++returnptr;+}++staticinlinevoid*ptr_ring_consume_bh(structptr_ring*r)+{+void*ptr;++spin_lock(&r->consumer_lock);+ptr=__ptr_ring_consume(r);+spin_unlock(&r->consumer_lock);++returnptr;+}++/* Cast to structure type and call a function without discarding from FIFO.+*Functionmustreturnavalue.+*Callersmusttakeconsumer_lock.+*/+#define __PTR_RING_PEEK_CALL(r, f) ((f)(__ptr_ring_peek(r)))++#define PTR_RING_PEEK_CALL(r, f) ({ \+typeof((f)(NULL))__PTR_RING_PEEK_CALL_v;\+\+spin_lock(&(r)->consumer_lock);\+__PTR_RING_PEEK_CALL_v=__PTR_RING_PEEK_CALL(r,f);\+spin_unlock(&(r)->consumer_lock);\+__PTR_RING_PEEK_CALL_v;\+})++#define PTR_RING_PEEK_CALL_IRQ(r, f) ({ \+typeof((f)(NULL))__PTR_RING_PEEK_CALL_v;\+\+spin_lock_irq(&(r)->consumer_lock);\+__PTR_RING_PEEK_CALL_v=__PTR_RING_PEEK_CALL(r,f);\+spin_unlock_irq(&(r)->consumer_lock);\+__PTR_RING_PEEK_CALL_v;\+})++#define PTR_RING_PEEK_CALL_BH(r, f) ({ \+typeof((f)(NULL))__PTR_RING_PEEK_CALL_v;\+\+spin_lock_bh(&(r)->consumer_lock);\+__PTR_RING_PEEK_CALL_v=__PTR_RING_PEEK_CALL(r,f);\+spin_unlock_bh(&(r)->consumer_lock);\+__PTR_RING_PEEK_CALL_v;\+})++#define PTR_RING_PEEK_CALL_ANY(r, f) ({ \+typeof((f)(NULL))__PTR_RING_PEEK_CALL_v;\+unsignedlong__PTR_RING_PEEK_CALL_f;\+\+spin_lock_irqsave(&(r)->consumer_lock,__PTR_RING_PEEK_CALL_f);\+__PTR_RING_PEEK_CALL_v=__PTR_RING_PEEK_CALL(r,f);\+spin_unlock_irqrestore(&(r)->consumer_lock,__PTR_RING_PEEK_CALL_f);\+__PTR_RING_PEEK_CALL_v;\+})++staticinlineintptr_ring_init(structptr_ring*r,intsize,gfp_tgfp)+{+r->queue=kzalloc(ALIGN(size*sizeof*(r->queue),SMP_CACHE_BYTES),+gfp);+if(!r->queue)+return-ENOMEM;++r->size=size;+r->producer=r->consumer=0;+spin_lock_init(&r->producer_lock);+spin_lock_init(&r->consumer_lock);++return0;+}++staticinlinevoidptr_ring_cleanup(structptr_ring*r)+{+kfree(r->queue);+}++#endif /* _LINUX_PTR_RING_H */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-06-02 16:08:34
A simple array based FIFO of pointers. Intended for net stack so uses
skbs for type safety. Implemented as a set of wrappers around ptr_array.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/skb_array.h | 143 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
create mode 100644 include/linux/skb_array.h
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-06-02 16:08:38
This adds ring resize support. Seems to be necessary as
users such as tun allow userspace control over queue size.
If resize is used, this costs us ability to peek at queue without
consumer lock - should not be a big deal as peek and consumer are
usually run on the same CPU.
If ring is made bigger, ring contents is preserved. If ring is made
smaller, extra pointers are passed to an optional destructor callback.
Cleanup function also gains destructor callback such that
all pointers in queue can be cleaned up.
This changes some APIs but we don't have any users yet,
so it won't break bisect.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/ptr_ring.h | 157 ++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 143 insertions(+), 14 deletions(-)
@@ -43,9 +43,9 @@ struct ptr_ring {};/* Note: callers invoking this in a loop must use a compiler barrier,-*forexamplecpu_relax().-*Callersdon'tneedtotakeproducerlock-iftheydon't-*thenextcallto__ptr_ring_producemayfail.+*forexamplecpu_relax().Ifringiseverresized,callersmusthold+*producer_lock-seee.g.ptr_ring_full.Otherwise,ifcallersdon'thold+*producer_lock,thenextcallto__ptr_ring_producemayfail.*/staticinlinebool__ptr_ring_full(structptr_ring*r){
@@ -54,16 +54,55 @@ static inline bool __ptr_ring_full(struct ptr_ring *r)staticinlineboolptr_ring_full(structptr_ring*r){-barrier();-return__ptr_ring_full(r);+boolret;++spin_lock(&r->producer_lock);+ret=__ptr_ring_full(r);+spin_unlock(&r->producer_lock);++returnret;+}++staticinlineboolptr_ring_full_irq(structptr_ring*r)+{+boolret;++spin_lock_irq(&r->producer_lock);+ret=__ptr_ring_full(r);+spin_unlock_irq(&r->producer_lock);++returnret;+}++staticinlineboolptr_ring_full_any(structptr_ring*r)+{+unsignedlongflags;+boolret;++spin_lock_irqsave(&r->producer_lock,flags);+ret=__ptr_ring_full(r);+spin_unlock_irqrestore(&r->producer_lock,flags);++returnret;+}++staticinlineboolptr_ring_full_bh(structptr_ring*r)+{+boolret;++spin_lock_bh(&r->producer_lock);+ret=__ptr_ring_full(r);+spin_unlock_bh(&r->producer_lock);++returnret;}/* Note: callers invoking this in a loop must use a compiler barrier,-*forexamplecpu_relax().+*forexamplecpu_relax().Callersmustholdproducer_lock.*/staticinlineint__ptr_ring_produce(structptr_ring*r,void*ptr){-if(__ptr_ring_full(r))+if(r->queue[r->producer])return-ENOSPC;r->queue[r->producer++]=ptr;
@@ -120,20 +159,68 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr)/* Note: callers invoking this in a loop must use a compiler barrier,*forexamplecpu_relax().Callersmusttakeconsumer_lock*iftheydereferencethepointer-seee.g.PTR_RING_PEEK_CALL.-*There'snoneedforalockifpointerismerelytested-seee.g.-*ptr_ring_empty.+*Ifringisneverresized,andifthepointerismerely+*tested,there'snoneedtotakethelock-seee.g.__ptr_ring_empty.*/staticinlinevoid*__ptr_ring_peek(structptr_ring*r){returnr->queue[r->consumer];}-staticinlineboolptr_ring_empty(structptr_ring*r)+/* Note: callers invoking this in a loop must use a compiler barrier,+*forexamplecpu_relax().Callersmusttakeconsumer_lock+*iftheringiseverresized-seee.g.ptr_ring_empty.+*/+staticinlinebool__ptr_ring_empty(structptr_ring*r){-barrier();return!__ptr_ring_peek(r);}+staticinlineboolptr_ring_empty(structptr_ring*r)+{+boolret;++spin_lock(&r->consumer_lock);+ret=__ptr_ring_empty(r);+spin_unlock(&r->consumer_lock);++returnret;+}++staticinlineboolptr_ring_empty_irq(structptr_ring*r)+{+boolret;++spin_lock_irq(&r->consumer_lock);+ret=__ptr_ring_empty(r);+spin_unlock_irq(&r->consumer_lock);++returnret;+}++staticinlineboolptr_ring_empty_any(structptr_ring*r)+{+unsignedlongflags;+boolret;++spin_lock_irqsave(&r->consumer_lock,flags);+ret=__ptr_ring_empty(r);+spin_unlock_irqrestore(&r->consumer_lock,flags);++returnret;+}++staticinlineboolptr_ring_empty_bh(structptr_ring*r)+{+boolret;++spin_lock_bh(&r->consumer_lock);+ret=__ptr_ring_empty(r);+spin_unlock_bh(&r->consumer_lock);++returnret;+}+/* Must only be called after __ptr_ring_peek returned !NULL */staticinlinevoid__ptr_ring_discard_one(structptr_ring*r){
@@ -62,9 +62,9 @@ static inline int skb_array_produce_any(struct skb_array *a, struct sk_buff *skbreturnptr_ring_produce_any(&a->ring,skb);}-/* Might be slightly faster than skb_array_empty below, but callers invoking-*thisinaloopmusttakecaretouseacompilerbarrier,forexample-*cpu_relax().+/* Might be slightly faster than skb_array_empty below, but only safe if the+*arrayisneverresized.Also,callersinvokingthisinaloopmusttakecare+*touseacompilerbarrier,forexamplecpu_relax().*/staticinlinebool__skb_array_empty(structskb_array*a){
Could not compile this new version of skb_array.h, it complains about
implicit declaration of function 'skb_vlan_tag_present' and
'VLAN_HLEN' being undeclared.
Fix this by including linux/if_vlan.h, but is that correct?
On Thu, 2 Jun 2016 19:08:26 +0300 "Michael S. Tsirkin" [off-list ref] wrote:
quoted hunk
A simple array based FIFO of pointers. Intended for net stack so uses
skbs for type safety. Implemented as a set of wrappers around ptr_array.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/skb_array.h | 143 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
create mode 100644 include/linux/skb_array.h
On Thu, 2 Jun 2016 19:08:18 +0300 "Michael S. Tsirkin" [off-list ref] wrote:
quoted hunk
A simple array based FIFO of pointers. Intended for net stack which
commonly has a single consumer/producer.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/ptr_ring.h | 264 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 264 insertions(+)
create mode 100644 include/linux/ptr_ring.h
My testing seems to show that at least for the common usecase
in networking, which isn't lockless, circular buffer
with indices does not perform that well, because
each index access causes a cache line to bounce between
CPUs, and index access causes stalls due to the dependency.
By comparison, an array of pointers where NULL means invalid
and !NULL means valid
As this might be useful outside of networking, I implemented
a generic array of void pointers, with a type-safe wrapper for skbs.
Nice
[...]
The only issue is with calls within a loop using the __ptr_ring_XXX
accessors - in theory compiler could hoist accesses out of the loop.
Following volatile-considered-harmful.txt I merely
documented that callers that busy-poll should invoke cpu_relax().
Most people will use the external skb_array_XXX APIs with a spinlock,
so this should not be an issue for them.