* [dpdk-dev] [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 [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: fix atomic refcnt update synchronization
2016-09-02 5:25 [dpdk-dev] [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: [dpdk-dev] [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: [dpdk-dev] [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: [dpdk-dev] [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: [dpdk-dev] [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 [dpdk-dev] [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 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).