[SPDK] SPDK Ring Enqueue/Dequeue Issue

[SPDK] SPDK Ring Enqueue/Dequeue Issue

[John Barnard]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

While testing our NVMf FC transport code, we ran into an issue with an
spdk_ring we are using to share an FC resource between multiple pollers
running on a particular FC port (i.e. a multi-producer, multi-consumer
scenario).  We were seeing corruption of the ring and the resources causing
the IO's to fail.  When we debugged this problem it came down to the
difference in the rte_ring calls being made but SPDK enqueue/dequeue.
Here's the code snippet from lib/env_dpdk/env.c (with the highlighted
difference):

size_t
spdk_ring_enqueue(struct spdk_ring *ring, void **objs, size_t count)
{
int rc;
#if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0)
rc = rte_ring_mp_enqueue_bulk((struct rte_ring *)ring, objs, count);
if (rc == 0) {
return count;
}

return 0;
#else
rc = rte_ring_mp_enqueue_bulk((struct rte_ring *)ring, objs, count, NULL);
return rc;
#endif
}

size_t
spdk_ring_dequeue(struct spdk_ring *ring, void **objs, size_t count)
{
#if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0)
return rte_ring_sc_dequeue_burst((struct rte_ring *)ring, objs, count);
#else
return rte_ring_sc_dequeue_burst((struct rte_ring *)ring, objs, count,
NULL);
#endif
}

It seems that the spdk_ring_enqueue function calls
rte_ring_mp_enqueue_bulk(), while spdk_ring_dequeue function calls
rte_ring_sc_dequeue_burst().  When we change it to call
ret_ring_mp_dequeue_burst(), the problem went away.  So my question is, why
this difference in the rte calls made by the SPDK?  Is this a bug or on
purpose?  Also, we noticed that there is no ring create flag (in
include/spdk/env.h) for multi-producer, multi-consumer (i.e.
SPDK_RING_TYPE_MP_MC), although it doesn't seem to matter if we pass the
SPDK_RING_TYPE_MP_SC flag to  spdk_ring_create() (i.e. it's the call to
rte_ring_mp_dequeue_burst that's critical).  Is there a reason this flag
was left out?
I'm not familiar with the history of these functions, but there seems to be
an inconsistency here and it's causing a failure for us.

Thanks,
John Barnard

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[Andrey Kuzmin]

[-- Attachment #1: Type: text/plain, Size: 2903 bytes --]

On Wed, Sep 19, 2018 at 9:13 PM John Barnard <john.barnard(a)broadcom.com>
wrote:

> While testing our NVMf FC transport code, we ran into an issue with an
> spdk_ring we are using to share an FC resource between multiple pollers
> running on a particular FC port (i.e. a multi-producer, multi-consumer
> scenario).  We were seeing corruption of the ring and the resources causing
> the IO's to fail.  When we debugged this problem it came down to the
> difference in the rte_ring calls being made but SPDK enqueue/dequeue.
> Here's the code snippet from lib/env_dpdk/env.c (with the highlighted
> difference):
>
> size_t
> spdk_ring_enqueue(struct spdk_ring *ring, void **objs, size_t count)
> {
> int rc;
> #if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0)
> rc = rte_ring_mp_enqueue_bulk((struct rte_ring *)ring, objs, count);
> if (rc == 0) {
> return count;
> }
>
> return 0;
> #else
> rc = rte_ring_mp_enqueue_bulk((struct rte_ring *)ring, objs, count, NULL);
> return rc;
> #endif
> }
>
> size_t
> spdk_ring_dequeue(struct spdk_ring *ring, void **objs, size_t count)
> {
> #if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0)
> return rte_ring_sc_dequeue_burst((struct rte_ring *)ring, objs, count);
> #else
> return rte_ring_sc_dequeue_burst((struct rte_ring *)ring, objs, count,
> NULL);
> #endif
> }
>
> It seems that the spdk_ring_enqueue function calls
> rte_ring_mp_enqueue_bulk(), while spdk_ring_dequeue function calls
> rte_ring_sc_dequeue_burst().  When we change it to call
> ret_ring_mp_dequeue_burst(), the problem went away.  So my question is, why
> this difference in the rte calls made by the SPDK?  Is this a bug or on
> purpose?  Also, we noticed that there is no ring create flag (in
> include/spdk/env.h) for multi-producer, multi-consumer (i.e.
> SPDK_RING_TYPE_MP_MC), although it doesn't seem to matter if we pass the
> SPDK_RING_TYPE_MP_SC flag to  spdk_ring_create() (i.e. it's the call to
> rte_ring_mp_dequeue_burst that's critical).  Is there a reason this flag
> was left out?
>

The two issues above are interrelated. SPDK seemingly does not care for
MP/MC rings, so it neither defines the flag to create the respective ring
type, nor appreciates this ring type under ring_dequeue (for enqueue, it
uses MP flavor which works for SP case as well).

The definitive answer as to why belongs to the SPDK team, while my guess is
that rings in SPDK are primarily (and solely, to the best of my knowledge)
for passing messages between SPDK threads where  MP/MC scenario does not
apply.

HTH,
Andrey


> I'm not familiar with the history of these functions, but there seems to be
> an inconsistency here and it's causing a failure for us.
>
> Thanks,
> John Barnard
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
>

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]



On 9/19/18, 11:37 AM, "SPDK on behalf of Andrey Kuzmin" <spdk-bounces(a)lists.01.org on behalf of andrey.v.kuzmin(a)gmail.com> wrote:

    On Wed, Sep 19, 2018 at 9:13 PM John Barnard <john.barnard(a)broadcom.com>
    wrote:
    
<snip>

    >
    > It seems that the spdk_ring_enqueue function calls
    > rte_ring_mp_enqueue_bulk(), while spdk_ring_dequeue function calls
    > rte_ring_sc_dequeue_burst().  When we change it to call
    > ret_ring_mp_dequeue_burst(), the problem went away.  So my question is, why
    > this difference in the rte calls made by the SPDK?  Is this a bug or on
    > purpose?  Also, we noticed that there is no ring create flag (in
    > include/spdk/env.h) for multi-producer, multi-consumer (i.e.
    > SPDK_RING_TYPE_MP_MC), although it doesn't seem to matter if we pass the
    > SPDK_RING_TYPE_MP_SC flag to  spdk_ring_create() (i.e. it's the call to
    > rte_ring_mp_dequeue_burst that's critical).  Is there a reason this flag
    > was left out?
    >
    
    The two issues above are interrelated. SPDK seemingly does not care for
    MP/MC rings, so it neither defines the flag to create the respective ring
    type, nor appreciates this ring type under ring_dequeue (for enqueue, it
    uses MP flavor which works for SP case as well).
    
    The definitive answer as to why belongs to the SPDK team, while my guess is
    that rings in SPDK are primarily (and solely, to the best of my knowledge)
    for passing messages between SPDK threads where  MP/MC scenario does not
  apply.

Andrey is correct.  There hasn’t been a use case for MP/MC rings in SPDK yet.  Rings are primarily used today for passing messages to a specific reactor – each reactor has its own ring (so it’s single consumer) but any thread may post messages to it.

I see no reason to not add MP/MC support if it’s needed for NVMe-oF FC.

John – is this something you’d like to tackle?

-Jim

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[Walker, Benjamin]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On Wed, 2018-09-19 at 19:07 +0000, Harris, James R wrote:
> 
> On 9/19/18, 11:37 AM, "SPDK on behalf of Andrey Kuzmin" <
> spdk-bounces(a)lists.01.org on behalf of andrey.v.kuzmin(a)gmail.com> wrote:
> 
>     On Wed, Sep 19, 2018 at 9:13 PM John Barnard <john.barnard(a)broadcom.com>
>     wrote:
>     
> <snip>
> 
>     >
>     > It seems that the spdk_ring_enqueue function calls
>     > rte_ring_mp_enqueue_bulk(), while spdk_ring_dequeue function calls
>     > rte_ring_sc_dequeue_burst().  When we change it to call
>     > ret_ring_mp_dequeue_burst(), the problem went away.  So my question is,
> why
>     > this difference in the rte calls made by the SPDK?  Is this a bug or on
>     > purpose?  Also, we noticed that there is no ring create flag (in
>     > include/spdk/env.h) for multi-producer, multi-consumer (i.e.
>     > SPDK_RING_TYPE_MP_MC), although it doesn't seem to matter if we pass the
>     > SPDK_RING_TYPE_MP_SC flag to  spdk_ring_create() (i.e. it's the call to
>     > rte_ring_mp_dequeue_burst that's critical).  Is there a reason this flag
>     > was left out?
>     >
>     
>     The two issues above are interrelated. SPDK seemingly does not care for
>     MP/MC rings, so it neither defines the flag to create the respective ring
>     type, nor appreciates this ring type under ring_dequeue (for enqueue, it
>     uses MP flavor which works for SP case as well).
>     
>     The definitive answer as to why belongs to the SPDK team, while my guess
> is
>     that rings in SPDK are primarily (and solely, to the best of my knowledge)
>     for passing messages between SPDK threads where  MP/MC scenario does not
>   apply.
> 
> Andrey is correct.  There hasn’t been a use case for MP/MC rings in SPDK
> yet.  Rings are primarily used today for passing messages to a specific
> reactor – each reactor has its own ring (so it’s single consumer) but any
> thread may post messages to it.
> 
> I see no reason to not add MP/MC support if it’s needed for NVMe-oF FC.

The best way to introduce this into SPDK is to add flags or function variants
for single/multi-consumer, so the appropriate DPDK function call can be used
underneath. Changing the ring from single consumer to multi-consumer introduces
an atomic instruction. That's why we avoid that, given our only use case today
is single consumer.

> 
> John – is this something you’d like to tackle?
> 
> -Jim
> 
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]



On 9/19/18, 12:42 PM, "SPDK on behalf of Walker, Benjamin" <spdk-bounces(a)lists.01.org on behalf of benjamin.walker(a)intel.com> wrote:

<snip>

    > 
    > I see no reason to not add MP/MC support if it’s needed for NVMe-oF FC.
    
    The best way to introduce this into SPDK is to add flags or function variants
    for single/multi-consumer, so the appropriate DPDK function call can be used
    underneath. Changing the ring from single consumer to multi-consumer introduces
    an atomic instruction. That's why we avoid that, given our only use case today
  is single consumer.

Exactly – just add a new SPDK_RING_TYPE_MP_MC value to enum spdk_ring_type, with the proper implementation in lib/env_dpdk.  Implementations for the existing ring types should not be modified.

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[John Barnard]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On Wed, Sep 19, 2018 at 12:48 PM Harris, James R <james.r.harris(a)intel.com>
wrote:

>
>
> On 9/19/18, 12:42 PM, "SPDK on behalf of Walker, Benjamin" <
> spdk-bounces(a)lists.01.org on behalf of benjamin.walker(a)intel.com> wrote:
>
> <snip>
>
>     >
>     > I see no reason to not add MP/MC support if it’s needed for NVMe-oF
> FC.
>
>     The best way to introduce this into SPDK is to add flags or function
> variants
>     for single/multi-consumer, so the appropriate DPDK function call can
> be used
>     underneath. Changing the ring from single consumer to multi-consumer
> introduces
>     an atomic instruction. That's why we avoid that, given our only use
> case today
>   is single consumer.
>
> Exactly – just add a new SPDK_RING_TYPE_MP_MC value to enum
> spdk_ring_type, with the proper implementation in lib/env_dpdk.
> Implementations for the existing ring types should not be modified.
>

So I found functions in  dpdk/lib/librte_ring/rte_ring.h:
rte_ring_enqueue() and rte_ring_dequeue(), which call the appropriate rte
functions based upon the rings flags.  It seems like I can just call them
directly from spdk_ring_enqueue() and spdk_ring_dequeue() as well as add
support for the new  SPDK_RING_TYPE_MP_MC flag.  I'm not sure what to do
about the #if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0) code in each of
those functions, which make slightly different rte calls.  I'm thinking
about leaving that alone since it's older and shouldn't be needed for older
apps running with this version of DPDK.  Tell me what you think?

Thanks,
John Barnard

>
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
>

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]



On 9/19/18, 2:14 PM, "SPDK on behalf of John Barnard" <spdk-bounces(a)lists.01.org on behalf of john.barnard(a)broadcom.com> wrote:

<snip>

    So I found functions in  dpdk/lib/librte_ring/rte_ring.h:
    rte_ring_enqueue() and rte_ring_dequeue(), which call the appropriate rte
    functions based upon the rings flags.  It seems like I can just call them
    directly from spdk_ring_enqueue() and spdk_ring_dequeue() as well as add
    support for the new  SPDK_RING_TYPE_MP_MC flag.  I'm not sure what to do
    about the #if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0) code in each of
    those functions, which make slightly different rte calls.  I'm thinking
    about leaving that alone since it's older and shouldn't be needed for older
    apps running with this version of DPDK.  Tell me what you think?

I think spdk_ring_enqueue/dequeue should just call rte_ring_enqueue/dequeue_bulk directly.

Calling the rte_ring mp/mc/sp/sc variants directly isn’t needed, even though that’s what the spdk_ring functions are doing today.  You’ll notice that if someone created a ring of type SPDK_RING_TYPE_SP_SC, we are ignoring the SP part and calling rte_ring_mp_enqueue_bulk anyways which is incorrect.  Nothing in the SPDK tree is using SPDK_RING_TYPE_SP_SC currently which is probably we haven’t noticed this until now.

I think you can just:

1) Add SPDK_RING_TYPE_MP_MC.
2) Modify spdk_ring_create() to handle this new ring type.
3) Modify spdk_ring_enqueue/dequeue() to call rte_ring_enqueue/dequeue_bulk() directly.

-Jim

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[John Barnard]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

And what about the code to support DPDK's older than 17.05?  Do we need to
even care about that anymore?  Can it be removed?  At minimum, can I leave
it as is?

Thanks,
John Barnard

On Wed, Sep 19, 2018 at 2:24 PM Harris, James R <james.r.harris(a)intel.com>
wrote:

>
>
> On 9/19/18, 2:14 PM, "SPDK on behalf of John Barnard" <
> spdk-bounces(a)lists.01.org on behalf of john.barnard(a)broadcom.com> wrote:
>
> <snip>
>
>     So I found functions in  dpdk/lib/librte_ring/rte_ring.h:
>     rte_ring_enqueue() and rte_ring_dequeue(), which call the appropriate
> rte
>     functions based upon the rings flags.  It seems like I can just call
> them
>     directly from spdk_ring_enqueue() and spdk_ring_dequeue() as well as
> add
>     support for the new  SPDK_RING_TYPE_MP_MC flag.  I'm not sure what to
> do
>     about the #if RTE_VERSION < RTE_VERSION_NUM(17, 5, 0, 0) code in each
> of
>     those functions, which make slightly different rte calls.  I'm thinking
>     about leaving that alone since it's older and shouldn't be needed for
> older
>     apps running with this version of DPDK.  Tell me what you think?
>
> I think spdk_ring_enqueue/dequeue should just call
> rte_ring_enqueue/dequeue_bulk directly.
>
> Calling the rte_ring mp/mc/sp/sc variants directly isn’t needed, even
> though that’s what the spdk_ring functions are doing today.  You’ll notice
> that if someone created a ring of type SPDK_RING_TYPE_SP_SC, we are
> ignoring the SP part and calling rte_ring_mp_enqueue_bulk anyways which is
> incorrect.  Nothing in the SPDK tree is using SPDK_RING_TYPE_SP_SC
> currently which is probably we haven’t noticed this until now.
>
> I think you can just:
>
> 1) Add SPDK_RING_TYPE_MP_MC.
> 2) Modify spdk_ring_create() to handle this new ring type.
> 3) Modify spdk_ring_enqueue/dequeue() to call
> rte_ring_enqueue/dequeue_bulk() directly.
>
> -Jim
>
>
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
>

Re: [SPDK] SPDK Ring Enqueue/Dequeue Issue

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]



On 9/19/18, 2:38 PM, "SPDK on behalf of John Barnard" <spdk-bounces(a)lists.01.org on behalf of john.barnard(a)broadcom.com> wrote:

    And what about the code to support DPDK's older than 17.05?  Do we need to
    even care about that anymore?  Can it be removed?  At minimum, can I leave
    it as is?

Hi John,

You can just make a similar change to the “older than 17.05” code – have it call the rte_ring_enqueue/dequeue_bulk function directly, just without that extra parameter.

-Jim