[dpdk-dev] mbuf: fix atomic refcnt update synchronization
Commit Message
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(-)
Comments
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.
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.
>
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?
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?
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?
@@ -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));
}