From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ig0-f182.google.com (mail-ig0-f182.google.com [209.85.213.182]) by dpdk.org (Postfix) with ESMTP id 4C09C58DF for ; Thu, 17 Mar 2016 16:48:44 +0100 (CET) Received: by mail-ig0-f182.google.com with SMTP id vf5so146441247igb.0 for ; Thu, 17 Mar 2016 08:48:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nofutznetworks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=pnvbQQWQzwgw72zFfW5nG8XvQCebJ9LXcWAWgD/GS6o=; b=S6Sv/WOLhN8Zg6w+2xXEm2Vy27iC4Rz8SV5Cb7twA5JW1d5/7xuhDw3iI7sfG/y3lt XY3no5p8hMNSGsRhV1aBsmPOfShun4aJyQ/CrMTvnltqJxVwUs3WQmke/2fdh+fv1IM5 LyNzqqb3ruHXjdareSzjme6/Fj/RVbYHWutaivVxS+FJAGQ8KoL1BN/Z7a1SIcKfWreh iGdf1P2EW9w3c/DGuGxIVaGF391HDTAU53AwoYdLQjJQn1dH0IDdRHukt8B+t679G0jq sqHizgtMwCBclG0GGj5Dc2S4PvVlrgWzSdctqjtL2MH1WPrnsOEDHlcL5GhP1reuGJn6 U33w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=pnvbQQWQzwgw72zFfW5nG8XvQCebJ9LXcWAWgD/GS6o=; b=hpJo5Op5NMfSOjLzTNtjtTlAdalQpy/rojn9k41SJb31cVv3Zdd2xjj9lQGZxuYldH d3knS0sIxvQlKYS3S+l7o7bj+tAybNBD8NkVyryaQHsnVGdl5BVjMoSnVWYsA6+OayNW kduhFNCnVojfC/flPx+FMgUde0LMAPs2Z+jNfmprJpcTkgEQKtdIj5UYEqViJQcT6Gm/ QPGIioZ9qRCBliE5OOGqt2BOnHi3QPhqKy56fdPfvXUItkxbwNV6FxAocAg/iZnUAjiP lSYnZenwcuKY/JJ1yYR3zxfFyrp4U3K3z5GGHVV+AbWMHQHfrhM0gxhcFBtqVFfl2nsT Gh1w== X-Gm-Message-State: AD7BkJJ06qm4tFj1j6WZ3DkhH0uNfuqL17eHWxvIl4/NLuqUKaybxUPWdkvmupyyJFX9axjdKU3zT5BUR/Tk1Q== MIME-Version: 1.0 X-Received: by 10.50.1.6 with SMTP id 6mr12573283igi.28.1458229723778; Thu, 17 Mar 2016 08:48:43 -0700 (PDT) Received: by 10.107.178.5 with HTTP; Thu, 17 Mar 2016 08:48:43 -0700 (PDT) In-Reply-To: <20160316112114.GC24668@bricha3-MOBL3> References: <1458061125-17515-1-git-send-email-l@nofutznetworks.com> <20160316112114.GC24668@bricha3-MOBL3> Date: Thu, 17 Mar 2016 17:48:43 +0200 Message-ID: From: Lazaros Koromilas To: Bruce Richardson Cc: dev@dpdk.org Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH] ring: assert on zero objects dequeue/enqueue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Mar 2016 15:48:44 -0000 Sure, I'm sending it again with your suggestions. Lazaros. On Wed, Mar 16, 2016 at 1:21 PM, Bruce Richardson wrote: > On Tue, Mar 15, 2016 at 06:58:45PM +0200, Lazaros Koromilas wrote: >> Issuing a zero objects dequeue with a single consumer has no effect. >> Doing so with multiple consumers, can get more than one thread to succeed >> the compare-and-set operation and observe starvation or even deadlock in >> the while loop that checks for preceding dequeues. The problematic piece >> of code when n = 0: >> >> cons_next = cons_head + n; >> success = rte_atomic32_cmpset(&r->cons.head, cons_head, cons_next); >> >> The same is possible on the enqueue path. >> >> Signed-off-by: Lazaros Koromilas > > I'm not sure how serious a problem this really is, and I really suspect that > just calling rte_panic is rather an overreaction here. At worst, this should > be a check only when RTE_RING_DEBUG is on. > > However, probably my preferred solution to this issue would be to just add > if (n == 0) > return 0 > > to the mp and mc enqueue/dequeue functions. That way there is no performance > penalty for the higher-performing sp/sc paths, and you avoid and unnecessary > cmpset operations for the mp/mc cases. > > /Bruce > >> --- >> lib/librte_ring/rte_ring.h | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h >> index 943c97c..2bf9ce3 100644 >> --- a/lib/librte_ring/rte_ring.h >> +++ b/lib/librte_ring/rte_ring.h >> @@ -100,6 +100,7 @@ extern "C" { >> #include >> #include >> #include >> +#include >> >> #define RTE_TAILQ_RING_NAME "RTE_RING" >> >> @@ -211,6 +212,19 @@ struct rte_ring { >> #endif >> >> /** >> + * @internal Assert macro. >> + * @param exp >> + * The expression to evaluate. >> + */ >> +#define RTE_RING_ASSERT(exp) do { \ >> + if (!(exp)) { \ >> + rte_panic("line%d\t" \ >> + "assert \"" #exp "\" failed\n", \ >> + __LINE__); \ >> + } \ >> + } while (0) >> + >> +/** >> * Calculate the memory size needed for a ring >> * >> * This function returns the number of bytes needed for a ring, given >> @@ -406,6 +420,7 @@ void rte_ring_dump(FILE *f, const struct rte_ring *r); >> * A pointer to a table of void * pointers (objects). >> * @param n >> * The number of objects to add in the ring from the obj_table. >> + * Must be greater than zero. >> * @param behavior >> * RTE_RING_QUEUE_FIXED: Enqueue a fixed number of items from a ring >> * RTE_RING_QUEUE_VARIABLE: Enqueue as many items a possible from ring >> @@ -431,6 +446,8 @@ __rte_ring_mp_do_enqueue(struct rte_ring *r, void * const *obj_table, >> uint32_t mask = r->prod.mask; >> int ret; >> >> + RTE_RING_ASSERT(n > 0); >> + >> /* move prod.head atomically */ >> do { >> /* Reset n to the initial burst count */ >> @@ -510,6 +527,7 @@ __rte_ring_mp_do_enqueue(struct rte_ring *r, void * const *obj_table, >> * A pointer to a table of void * pointers (objects). >> * @param n >> * The number of objects to add in the ring from the obj_table. >> + * Must be greater than zero. >> * @param behavior >> * RTE_RING_QUEUE_FIXED: Enqueue a fixed number of items from a ring >> * RTE_RING_QUEUE_VARIABLE: Enqueue as many items a possible from ring >> @@ -533,6 +551,8 @@ __rte_ring_sp_do_enqueue(struct rte_ring *r, void * const *obj_table, >> uint32_t mask = r->prod.mask; >> int ret; >> >> + RTE_RING_ASSERT(n > 0); >> + >> prod_head = r->prod.head; >> cons_tail = r->cons.tail; >> /* The subtraction is done between two unsigned 32bits value >> @@ -594,6 +614,7 @@ __rte_ring_sp_do_enqueue(struct rte_ring *r, void * const *obj_table, >> * A pointer to a table of void * pointers (objects) that will be filled. >> * @param n >> * The number of objects to dequeue from the ring to the obj_table. >> + * Must be greater than zero. >> * @param behavior >> * RTE_RING_QUEUE_FIXED: Dequeue a fixed number of items from a ring >> * RTE_RING_QUEUE_VARIABLE: Dequeue as many items a possible from ring >> @@ -618,6 +639,8 @@ __rte_ring_mc_do_dequeue(struct rte_ring *r, void **obj_table, >> unsigned i, rep = 0; >> uint32_t mask = r->prod.mask; >> >> + RTE_RING_ASSERT(n > 0); >> + >> /* move cons.head atomically */ >> do { >> /* Restore n as it may change every loop */ >> @@ -689,6 +712,7 @@ __rte_ring_mc_do_dequeue(struct rte_ring *r, void **obj_table, >> * A pointer to a table of void * pointers (objects) that will be filled. >> * @param n >> * The number of objects to dequeue from the ring to the obj_table. >> + * Must be greater than zero. >> * @param behavior >> * RTE_RING_QUEUE_FIXED: Dequeue a fixed number of items from a ring >> * RTE_RING_QUEUE_VARIABLE: Dequeue as many items a possible from ring >> @@ -710,6 +734,8 @@ __rte_ring_sc_do_dequeue(struct rte_ring *r, void **obj_table, >> unsigned i; >> uint32_t mask = r->prod.mask; >> >> + RTE_RING_ASSERT(n > 0); >> + >> cons_head = r->cons.head; >> prod_tail = r->prod.tail; >> /* The subtraction is done between two unsigned 32bits value >> -- >> 1.9.1 >>