All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mbuf: fix atomic refcnt update synchronization
@ 2016-09-02  5:25 lilinzhe
  2016-09-02 16:12 ` Stephen Hemminger
  0 siblings, 1 reply; 6+ messages in thread
From: lilinzhe @ 2016-09-02  5:25 UTC (permalink / raw)
  To: dev; +Cc: 李林哲

From: 李林哲 <lilinzhe@ijinshan.com>

chagne atomic ref update to always call atomic_add

when mbuf is allocated by cpu1 and freed by cpu2. cpu1 cache may not be updated by such a set operation.
causes refcnt reads incorrect values.
---
 lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7ea66ed..63e6588 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -786,7 +786,7 @@ struct rte_mbuf {
 	 */
 	union {
 		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
-		uint16_t refcnt;              /**< Non-atomically accessed refcnt */
+		volatile uint16_t refcnt;     /**< Non-atomically accessed refcnt */
 	};
 	uint8_t nb_segs;          /**< Number of segments. */
 	uint8_t port;             /**< Input port. */
@@ -1060,16 +1060,12 @@ static inline uint16_t
 rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
 	/*
-	 * The atomic_add is an expensive operation, so we don't want to
-	 * call it in the case where we know we are the uniq holder of
-	 * this mbuf (i.e. ref_cnt == 1). Otherwise, an atomic
-	 * operation has to be used because concurrent accesses on the
-	 * reference counter can occur.
+	 * This shell always call atomic_add
+	 *
+	 * when mbuf is allocated by cpu1 and freed by cpu2. cpu1 cache may not be updated by such a set operation.
+	 * causes refcnt reads incorrect values
+	 * 
 	 */
-	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
-		rte_mbuf_refcnt_set(m, 1 + value);
-		return 1 + value;
-	}
 
 	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
 }
-- 
2.7.0.rc2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mbuf: fix atomic refcnt update synchronization
  2016-09-02  5:25 [PATCH] mbuf: fix atomic refcnt update synchronization lilinzhe
@ 2016-09-02 16:12 ` Stephen Hemminger
  2016-09-02 16:31   ` Linzhe Lee
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2016-09-02 16:12 UTC (permalink / raw)
  To: lilinzhe; +Cc: dev, 李林哲

On Fri,  2 Sep 2016 13:25:06 +0800
lilinzhe <slayercat.subscription@gmail.com> wrote:

> From: 李林哲 <lilinzhe@ijinshan.com>
> 
> chagne atomic ref update to always call atomic_add
> 
> when mbuf is allocated by cpu1 and freed by cpu2. cpu1 cache may not be updated by such a set operation.
> causes refcnt reads incorrect values.

What architecture are you dealing with? On X86 memory is cache coherent.

Doing atomic operation all the time on each mbuf free would significantly
slow down performance.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mbuf: fix atomic refcnt update synchronization
  2016-09-02 16:12 ` Stephen Hemminger
@ 2016-09-02 16:31   ` Linzhe Lee
  2016-09-02 16:51     ` Stephen Hemminger
  0 siblings, 1 reply; 6+ messages in thread
From: Linzhe Lee @ 2016-09-02 16:31 UTC (permalink / raw)
  To: Stephen Hemminger, dev

Thanks for reply, Stephen.



I'm in x86-64, my cpu is `Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz`.



When allocation mbuf in program1, and transfer it to program2 for free
via ring, the program1 might meet assert in allocate mbuf sometimes.
(`RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);`)



but when I using gdb to check it, the refcnt field of mbuf is already
zero. so I believe the problem came from the cache line problem or
incorrect optimization.



When apply this patch, the problem seems solved. I'm submitting it for
your comments.


2016-09-03 0:12 GMT+08:00 Stephen Hemminger <stephen@networkplumber.org>:
> On Fri,  2 Sep 2016 13:25:06 +0800
> lilinzhe <slayercat.subscription@gmail.com> wrote:
>
>> From: 李林哲 <lilinzhe@ijinshan.com>
>>
>> chagne atomic ref update to always call atomic_add
>>
>> when mbuf is allocated by cpu1 and freed by cpu2. cpu1 cache may not be updated by such a set operation.
>> causes refcnt reads incorrect values.
>
> What architecture are you dealing with? On X86 memory is cache coherent.
>
> Doing atomic operation all the time on each mbuf free would significantly
> slow down performance.
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mbuf: fix atomic refcnt update synchronization
  2016-09-02 16:31   ` Linzhe Lee
@ 2016-09-02 16:51     ` Stephen Hemminger
  2016-09-03  2:05       ` Linzhe Lee
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2016-09-02 16:51 UTC (permalink / raw)
  To: Linzhe Lee; +Cc: dev

On Sat, 3 Sep 2016 00:31:50 +0800
Linzhe Lee <slayercat.subscription@gmail.com> wrote:

> Thanks for reply, Stephen.
> 
> 
> 
> I'm in x86-64, my cpu is `Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz`.
> 
> 
> 
> When allocation mbuf in program1, and transfer it to program2 for free
> via ring, the program1 might meet assert in allocate mbuf sometimes.
> (`RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);`)
> 
> 
> 
> but when I using gdb to check it, the refcnt field of mbuf is already
> zero. so I believe the problem came from the cache line problem or
> incorrect optimization.
> 
> 
> 
> When apply this patch, the problem seems solved. I'm submitting it for
> your comments.

Are you sure you have REFCNT_ATOMIC set?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mbuf: fix atomic refcnt update synchronization
  2016-09-02 16:51     ` Stephen Hemminger
@ 2016-09-03  2:05       ` Linzhe Lee
  2016-09-03 22:52         ` Ananyev, Konstantin
  0 siblings, 1 reply; 6+ messages in thread
From: Linzhe Lee @ 2016-09-03  2:05 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

yes,stephen.

my config file here: http://pastebin.com/N0RKGArh

2016-09-03 0:51 GMT+08:00 Stephen Hemminger <stephen@networkplumber.org>:
> On Sat, 3 Sep 2016 00:31:50 +0800
> Linzhe Lee <slayercat.subscription@gmail.com> wrote:
>
>> Thanks for reply, Stephen.
>>
>>
>>
>> I'm in x86-64, my cpu is `Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz`.
>>
>>
>>
>> When allocation mbuf in program1, and transfer it to program2 for free
>> via ring, the program1 might meet assert in allocate mbuf sometimes.
>> (`RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);`)
>>
>>
>>
>> but when I using gdb to check it, the refcnt field of mbuf is already
>> zero. so I believe the problem came from the cache line problem or
>> incorrect optimization.
>>
>>
>>
>> When apply this patch, the problem seems solved. I'm submitting it for
>> your comments.
>
> Are you sure you have REFCNT_ATOMIC set?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mbuf: fix atomic refcnt update synchronization
  2016-09-03  2:05       ` Linzhe Lee
@ 2016-09-03 22:52         ` Ananyev, Konstantin
  0 siblings, 0 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2016-09-03 22:52 UTC (permalink / raw)
  To: Linzhe Lee, Stephen Hemminger; +Cc: dev

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Linzhe Lee
> Sent: Saturday, September 3, 2016 3:05 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mbuf: fix atomic refcnt update synchronization
> 
> yes,stephen.
> 
> my config file here: http://pastebin.com/N0RKGArh
> 
> 2016-09-03 0:51 GMT+08:00 Stephen Hemminger <stephen@networkplumber.org>:
> > On Sat, 3 Sep 2016 00:31:50 +0800
> > Linzhe Lee <slayercat.subscription@gmail.com> wrote:
> >
> >> Thanks for reply, Stephen.
> >>
> >>
> >>
> >> I'm in x86-64, my cpu is `Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz`.
> >>
> >>
> >>
> >> When allocation mbuf in program1, and transfer it to program2 for
> >> free via ring, the program1 might meet assert in allocate mbuf sometimes.
> >> (`RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);`)

If you believe there is a problem inside rte_mbuf code,
please provide a test program to reproduce the issue.
So far, I personally don't see any issue in the rte_mbuf code.
Konstantin


> >>
> >>
> >>
> >> but when I using gdb to check it, the refcnt field of mbuf is already
> >> zero. so I believe the problem came from the cache line problem or
> >> incorrect optimization.
> >>
> >>
> >>
> >> When apply this patch, the problem seems solved. I'm submitting it
> >> for your comments.
> >
> > Are you sure you have REFCNT_ATOMIC set?

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-09-03 22:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-02  5:25 [PATCH] mbuf: fix atomic refcnt update synchronization lilinzhe
2016-09-02 16:12 ` Stephen Hemminger
2016-09-02 16:31   ` Linzhe Lee
2016-09-02 16:51     ` Stephen Hemminger
2016-09-03  2:05       ` Linzhe Lee
2016-09-03 22:52         ` Ananyev, Konstantin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.