* [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume
@ 2017-04-24 16:01 Michael S. Tsirkin
2017-04-26 9:09 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-04-24 16:01 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang
Applications that consume a batch of entries in one go
can benefit from ability to return some of them back
into the ring.
Add an API for that - assuming there's space. If there's no space
naturally can't do this and have to drop entries, but this implies ring
is full so we'd likely drop some anyway.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Jason, if you add this and unconsume the outstanding packets
on backend disconnect, vhost close and reset, I think
we should apply your patch even if we don't yet know 100%
why it helps.
changes from v1:
- fix up coding style issues reported by Sergei Shtylyov
include/linux/ptr_ring.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 783e7f5..902afc2 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -457,6 +457,62 @@ static inline int ptr_ring_init(struct ptr_ring *r, int size, gfp_t gfp)
return 0;
}
+/*
+ * Return entries into ring. Destroy entries that don't fit.
+ *
+ * Note: this is expected to be a rare slow path operation.
+ *
+ * Note: producer lock is nested within consumer lock, so if you
+ * resize you must make sure all uses nest correctly.
+ * In particular if you consume ring in interrupt or BH context, you must
+ * disable interrupts/BH when doing so.
+ */
+static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
+ void (*destroy)(void *))
+{
+ unsigned long flags;
+ int head;
+
+ spin_lock_irqsave(&r->consumer_lock, flags);
+ spin_lock(&r->producer_lock);
+
+ if (!r->size)
+ goto done;
+
+ /*
+ * Clean out buffered entries (for simplicity). This way following code
+ * can test entries for NULL and if not assume they are valid.
+ */
+ head = r->consumer_head - 1;
+ while (likely(head >= r->consumer_tail))
+ r->queue[head--] = NULL;
+ r->consumer_tail = r->consumer_head;
+
+ /*
+ * Go over entries in batch, start moving head back and copy entries.
+ * Stop when we run into previously unconsumed entries.
+ */
+ while (n--) {
+ head = r->consumer_head - 1;
+ if (head < 0)
+ head = r->size - 1;
+ if (r->queue[head]) {
+ /* This batch entry will have to be destroyed. */
+ ++n;
+ goto done;
+ }
+ r->queue[head] = batch[n];
+ r->consumer_tail = r->consumer_head = head;
+ }
+
+done:
+ /* Destroy all entries left in the batch. */
+ while (n--)
+ destroy(batch[n]);
+ spin_unlock(&r->producer_lock);
+ spin_unlock_irqrestore(&r->consumer_lock, flags);
+}
+
static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
int size, gfp_t gfp,
void (*destroy)(void *))
--
MST
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume
2017-04-24 16:01 [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume Michael S. Tsirkin
@ 2017-04-26 9:09 ` Jason Wang
2017-04-26 15:54 ` Michael S. Tsirkin
2017-05-09 13:26 ` Michael S. Tsirkin
0 siblings, 2 replies; 5+ messages in thread
From: Jason Wang @ 2017-04-26 9:09 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev
On 2017年04月25日 00:01, Michael S. Tsirkin wrote:
> Applications that consume a batch of entries in one go
> can benefit from ability to return some of them back
> into the ring.
>
> Add an API for that - assuming there's space. If there's no space
> naturally can't do this and have to drop entries, but this implies ring
> is full so we'd likely drop some anyway.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Jason, if you add this and unconsume the outstanding packets
> on backend disconnect, vhost close and reset, I think
> we should apply your patch even if we don't yet know 100%
> why it helps.
>
> changes from v1:
> - fix up coding style issues reported by Sergei Shtylyov
>
>
> include/linux/ptr_ring.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 783e7f5..902afc2 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -457,6 +457,62 @@ static inline int ptr_ring_init(struct ptr_ring *r, int size, gfp_t gfp)
> return 0;
> }
>
> +/*
> + * Return entries into ring. Destroy entries that don't fit.
> + *
> + * Note: this is expected to be a rare slow path operation.
> + *
> + * Note: producer lock is nested within consumer lock, so if you
> + * resize you must make sure all uses nest correctly.
> + * In particular if you consume ring in interrupt or BH context, you must
> + * disable interrupts/BH when doing so.
> + */
> +static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
> + void (*destroy)(void *))
> +{
> + unsigned long flags;
> + int head;
> +
> + spin_lock_irqsave(&r->consumer_lock, flags);
> + spin_lock(&r->producer_lock);
> +
> + if (!r->size)
> + goto done;
> +
> + /*
> + * Clean out buffered entries (for simplicity). This way following code
> + * can test entries for NULL and if not assume they are valid.
> + */
> + head = r->consumer_head - 1;
> + while (likely(head >= r->consumer_tail))
> + r->queue[head--] = NULL;
> + r->consumer_tail = r->consumer_head;
> +
> + /*
> + * Go over entries in batch, start moving head back and copy entries.
> + * Stop when we run into previously unconsumed entries.
> + */
> + while (n--) {
> + head = r->consumer_head - 1;
> + if (head < 0)
> + head = r->size - 1;
> + if (r->queue[head]) {
> + /* This batch entry will have to be destroyed. */
> + ++n;
> + goto done;
> + }
> + r->queue[head] = batch[n];
> + r->consumer_tail = r->consumer_head = head;
Looks like something wrong here (bad page state reported), uncomment the
above while() solving the issue. But after staring it for a while I
didn't find anything interesting, maybe you have some idea on this?
Thanks
> + }
> +
> +done:
> + /* Destroy all entries left in the batch. */
> + while (n--)
> + destroy(batch[n]);
> + spin_unlock(&r->producer_lock);
> + spin_unlock_irqrestore(&r->consumer_lock, flags);
> +}
> +
> static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
> int size, gfp_t gfp,
> void (*destroy)(void *))
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume
2017-04-26 9:09 ` Jason Wang
@ 2017-04-26 15:54 ` Michael S. Tsirkin
2017-05-09 13:26 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-04-26 15:54 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev
On Wed, Apr 26, 2017 at 05:09:42PM +0800, Jason Wang wrote:
>
>
> On 2017年04月25日 00:01, Michael S. Tsirkin wrote:
> > Applications that consume a batch of entries in one go
> > can benefit from ability to return some of them back
> > into the ring.
> >
> > Add an API for that - assuming there's space. If there's no space
> > naturally can't do this and have to drop entries, but this implies ring
> > is full so we'd likely drop some anyway.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Jason, if you add this and unconsume the outstanding packets
> > on backend disconnect, vhost close and reset, I think
> > we should apply your patch even if we don't yet know 100%
> > why it helps.
> >
> > changes from v1:
> > - fix up coding style issues reported by Sergei Shtylyov
> >
> >
> > include/linux/ptr_ring.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > index 783e7f5..902afc2 100644
> > --- a/include/linux/ptr_ring.h
> > +++ b/include/linux/ptr_ring.h
> > @@ -457,6 +457,62 @@ static inline int ptr_ring_init(struct ptr_ring *r, int size, gfp_t gfp)
> > return 0;
> > }
> > +/*
> > + * Return entries into ring. Destroy entries that don't fit.
> > + *
> > + * Note: this is expected to be a rare slow path operation.
> > + *
> > + * Note: producer lock is nested within consumer lock, so if you
> > + * resize you must make sure all uses nest correctly.
> > + * In particular if you consume ring in interrupt or BH context, you must
> > + * disable interrupts/BH when doing so.
> > + */
> > +static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
> > + void (*destroy)(void *))
> > +{
> > + unsigned long flags;
> > + int head;
> > +
> > + spin_lock_irqsave(&r->consumer_lock, flags);
> > + spin_lock(&r->producer_lock);
> > +
> > + if (!r->size)
> > + goto done;
> > +
> > + /*
> > + * Clean out buffered entries (for simplicity). This way following code
> > + * can test entries for NULL and if not assume they are valid.
> > + */
> > + head = r->consumer_head - 1;
> > + while (likely(head >= r->consumer_tail))
> > + r->queue[head--] = NULL;
> > + r->consumer_tail = r->consumer_head;
> > +
> > + /*
> > + * Go over entries in batch, start moving head back and copy entries.
> > + * Stop when we run into previously unconsumed entries.
> > + */
> > + while (n--) {
Maybe this is called with n = 0?
Should be while (n-- > 0) I guess so n = 0 is valid.
> > + head = r->consumer_head - 1;
> > + if (head < 0)
> > + head = r->size - 1;
> > + if (r->queue[head]) {
> > + /* This batch entry will have to be destroyed. */
> > + ++n;
> > + goto done;
> > + }
> > + r->queue[head] = batch[n];
> > + r->consumer_tail = r->consumer_head = head;
>
> Looks like something wrong here (bad page state reported), uncomment the
> above while()
I guess you mean comment out?
> solving the issue. But after staring it for a while I didn't
> find anything interesting, maybe you have some idea on this?
>
> Thanks
Add tracing to see what's going on?
>
> > + }
> > +
> > +done:
> > + /* Destroy all entries left in the batch. */
> > + while (n--)
> > + destroy(batch[n]);
> > + spin_unlock(&r->producer_lock);
> > + spin_unlock_irqrestore(&r->consumer_lock, flags);
> > +}
> > +
> > static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
> > int size, gfp_t gfp,
> > void (*destroy)(void *))
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume
2017-04-26 9:09 ` Jason Wang
2017-04-26 15:54 ` Michael S. Tsirkin
@ 2017-05-09 13:26 ` Michael S. Tsirkin
2017-05-10 2:01 ` Jason Wang
1 sibling, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-05-09 13:26 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev
On Wed, Apr 26, 2017 at 05:09:42PM +0800, Jason Wang wrote:
>
>
> On 2017年04月25日 00:01, Michael S. Tsirkin wrote:
> > Applications that consume a batch of entries in one go
> > can benefit from ability to return some of them back
> > into the ring.
> >
> > Add an API for that - assuming there's space. If there's no space
> > naturally can't do this and have to drop entries, but this implies ring
> > is full so we'd likely drop some anyway.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Jason, if you add this and unconsume the outstanding packets
> > on backend disconnect, vhost close and reset, I think
> > we should apply your patch even if we don't yet know 100%
> > why it helps.
> >
> > changes from v1:
> > - fix up coding style issues reported by Sergei Shtylyov
> >
> >
> > include/linux/ptr_ring.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > index 783e7f5..902afc2 100644
> > --- a/include/linux/ptr_ring.h
> > +++ b/include/linux/ptr_ring.h
> > @@ -457,6 +457,62 @@ static inline int ptr_ring_init(struct ptr_ring *r, int size, gfp_t gfp)
> > return 0;
> > }
> > +/*
> > + * Return entries into ring. Destroy entries that don't fit.
> > + *
> > + * Note: this is expected to be a rare slow path operation.
> > + *
> > + * Note: producer lock is nested within consumer lock, so if you
> > + * resize you must make sure all uses nest correctly.
> > + * In particular if you consume ring in interrupt or BH context, you must
> > + * disable interrupts/BH when doing so.
> > + */
> > +static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
> > + void (*destroy)(void *))
> > +{
> > + unsigned long flags;
> > + int head;
> > +
> > + spin_lock_irqsave(&r->consumer_lock, flags);
> > + spin_lock(&r->producer_lock);
> > +
> > + if (!r->size)
> > + goto done;
> > +
> > + /*
> > + * Clean out buffered entries (for simplicity). This way following code
> > + * can test entries for NULL and if not assume they are valid.
> > + */
> > + head = r->consumer_head - 1;
> > + while (likely(head >= r->consumer_tail))
> > + r->queue[head--] = NULL;
> > + r->consumer_tail = r->consumer_head;
> > +
> > + /*
> > + * Go over entries in batch, start moving head back and copy entries.
> > + * Stop when we run into previously unconsumed entries.
> > + */
> > + while (n--) {
> > + head = r->consumer_head - 1;
> > + if (head < 0)
> > + head = r->size - 1;
> > + if (r->queue[head]) {
> > + /* This batch entry will have to be destroyed. */
> > + ++n;
> > + goto done;
> > + }
> > + r->queue[head] = batch[n];
> > + r->consumer_tail = r->consumer_head = head;
>
> Looks like something wrong here (bad page state reported), uncomment the
> above while() solving the issue. But after staring it for a while I didn't
> find anything interesting, maybe you have some idea on this?
>
> Thanks
>
>
> > + }
> > +
> > +done:
> > + /* Destroy all entries left in the batch. */
> > + while (n--)
> > + destroy(batch[n]);
> > + spin_unlock(&r->producer_lock);
> > + spin_unlock_irqrestore(&r->consumer_lock, flags);
> > +}
> > +
> > static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
> > int size, gfp_t gfp,
> > void (*destroy)(void *))
What's our plan here? I can't delay pull request much longer.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume
2017-05-09 13:26 ` Michael S. Tsirkin
@ 2017-05-10 2:01 ` Jason Wang
0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2017-05-10 2:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, netdev
On 2017年05月09日 21:26, Michael S. Tsirkin wrote:
> On Wed, Apr 26, 2017 at 05:09:42PM +0800, Jason Wang wrote:
>>
>> On 2017年04月25日 00:01, Michael S. Tsirkin wrote:
>>> Applications that consume a batch of entries in one go
>>> can benefit from ability to return some of them back
>>> into the ring.
>>>
>>> Add an API for that - assuming there's space. If there's no space
>>> naturally can't do this and have to drop entries, but this implies ring
>>> is full so we'd likely drop some anyway.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>
>>> Jason, if you add this and unconsume the outstanding packets
>>> on backend disconnect, vhost close and reset, I think
>>> we should apply your patch even if we don't yet know 100%
>>> why it helps.
>>>
>>> changes from v1:
>>> - fix up coding style issues reported by Sergei Shtylyov
>>>
>>>
>>> include/linux/ptr_ring.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 56 insertions(+)
>>>
>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>>> index 783e7f5..902afc2 100644
>>> --- a/include/linux/ptr_ring.h
>>> +++ b/include/linux/ptr_ring.h
>>> @@ -457,6 +457,62 @@ static inline int ptr_ring_init(struct ptr_ring *r, int size, gfp_t gfp)
>>> return 0;
>>> }
>>> +/*
>>> + * Return entries into ring. Destroy entries that don't fit.
>>> + *
>>> + * Note: this is expected to be a rare slow path operation.
>>> + *
>>> + * Note: producer lock is nested within consumer lock, so if you
>>> + * resize you must make sure all uses nest correctly.
>>> + * In particular if you consume ring in interrupt or BH context, you must
>>> + * disable interrupts/BH when doing so.
>>> + */
>>> +static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
>>> + void (*destroy)(void *))
>>> +{
>>> + unsigned long flags;
>>> + int head;
>>> +
>>> + spin_lock_irqsave(&r->consumer_lock, flags);
>>> + spin_lock(&r->producer_lock);
>>> +
>>> + if (!r->size)
>>> + goto done;
>>> +
>>> + /*
>>> + * Clean out buffered entries (for simplicity). This way following code
>>> + * can test entries for NULL and if not assume they are valid.
>>> + */
>>> + head = r->consumer_head - 1;
>>> + while (likely(head >= r->consumer_tail))
>>> + r->queue[head--] = NULL;
>>> + r->consumer_tail = r->consumer_head;
>>> +
>>> + /*
>>> + * Go over entries in batch, start moving head back and copy entries.
>>> + * Stop when we run into previously unconsumed entries.
>>> + */
>>> + while (n--) {
>>> + head = r->consumer_head - 1;
>>> + if (head < 0)
>>> + head = r->size - 1;
>>> + if (r->queue[head]) {
>>> + /* This batch entry will have to be destroyed. */
>>> + ++n;
>>> + goto done;
>>> + }
>>> + r->queue[head] = batch[n];
>>> + r->consumer_tail = r->consumer_head = head;
>> Looks like something wrong here (bad page state reported), uncomment the
>> above while() solving the issue. But after staring it for a while I didn't
>> find anything interesting, maybe you have some idea on this?
>>
>> Thanks
>>
>>
>>> + }
>>> +
>>> +done:
>>> + /* Destroy all entries left in the batch. */
>>> + while (n--)
>>> + destroy(batch[n]);
>>> + spin_unlock(&r->producer_lock);
>>> + spin_unlock_irqrestore(&r->consumer_lock, flags);
>>> +}
>>> +
>>> static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
>>> int size, gfp_t gfp,
>>> void (*destroy)(void *))
> What's our plan here? I can't delay pull request much longer.
>
I'm waiting for net-next to be opened (since the series touches tun/tap).
Let me post a new version soon.
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-10 2:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-24 16:01 [PATCH RFC v2] ptr_ring: add ptr_ring_unconsume Michael S. Tsirkin
2017-04-26 9:09 ` Jason Wang
2017-04-26 15:54 ` Michael S. Tsirkin
2017-05-09 13:26 ` Michael S. Tsirkin
2017-05-10 2:01 ` Jason Wang
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).