[dpdk-dev] [memnic PATCH 1/5] pmd: fix race condition
2014-03-28 09:49, Hiroshi Shimamoto: > Do you want me resubmit update one? > If so, will do next week. Yes, please submit a v2. Thank you -- Thomas
[dpdk-dev] [memnic PATCH 1/5] pmd: fix race condition
Hi, > Subject: Re: [dpdk-dev] [memnic PATCH 1/5] pmd: fix race condition > > Hi Hiroshi-san, > > Please see my comments below. > > On 03/11/2014 06:37 AM, Hiroshi Shimamoto wrote: > > From: Hiroshi Shimamoto > > > > There is a race condition, on transmit to vSwitch. > > I think we should not talk specifically about vSwitch, as > another implementation of host memnic is possible. Maybe using > the term "host" is more appropriate? > > > + if (idx != ACCESS_ONCE(adapter->down_idx)) { > > + /* > > +* vSwitch freed this and got false positive, > > +* need to recover the status and retry. > > +*/ > > + p->status = MEMNIC_PKT_ST_FREE; > > + goto retry; > > + } > > + > > The patch indeed looks to improve reliability, even if it's > difficult to me to be sure that there is no other race condition. > Again, I would replace "vSwitch" by "host". okay, I'm fine with that. Do you want me resubmit update one? If so, will do next week. > > By the way, I guess the Linux code in linux/memnic_net.c should be > modified in the same way. Hm, yes, we should check kernel driver too. thanks, Hiroshi > > Regards, > Olivier
[dpdk-dev] [memnic PATCH 1/5] pmd: fix race condition
Hi Hiroshi-san, Please see my comments below. On 03/11/2014 06:37 AM, Hiroshi Shimamoto wrote: > From: Hiroshi Shimamoto > > There is a race condition, on transmit to vSwitch. I think we should not talk specifically about vSwitch, as another implementation of host memnic is possible. Maybe using the term "host" is more appropriate? > + if (idx != ACCESS_ONCE(adapter->down_idx)) { > + /* > + * vSwitch freed this and got false positive, > + * need to recover the status and retry. > + */ > + p->status = MEMNIC_PKT_ST_FREE; > + goto retry; > + } > + The patch indeed looks to improve reliability, even if it's difficult to me to be sure that there is no other race condition. Again, I would replace "vSwitch" by "host". By the way, I guess the Linux code in linux/memnic_net.c should be modified in the same way. Regards, Olivier
[dpdk-dev] [memnic PATCH 1/5] pmd: fix race condition
From: Hiroshi Shimamoto There is a race condition, on transmit to vSwitch. Guest PMD Host Thread-A Thread-B vSwitch |idx=0 |idx=0 |p[0] st!=2 |cmpxchg || |p[0] st->1 || |idx=1 || |fill data || |p[0] st->2 ||p[0] st==2 |||receive data |||p[0] st->0 ||cmpxchg | ||success |p[1] st!=2 ||p[0] st->1 | This is BAD That causes traffic stop. We have to take care about that race condition with checking whether current index is correct. Signed-off-by: Hiroshi Shimamoto Reviewed-by: Hayato Momma --- pmd/pmd_memnic.c | 9 + 1 file changed, 9 insertions(+) diff --git a/pmd/pmd_memnic.c b/pmd/pmd_memnic.c index 30d5a1b..805f0b2 100644 --- a/pmd/pmd_memnic.c +++ b/pmd/pmd_memnic.c @@ -278,6 +278,15 @@ retry: goto retry; } + if (idx != ACCESS_ONCE(adapter->down_idx)) { + /* +* vSwitch freed this and got false positive, +* need to recover the status and retry. +*/ + p->status = MEMNIC_PKT_ST_FREE; + goto retry; + } + if (++idx >= MEMNIC_NR_PACKET) idx = 0; adapter->down_idx = idx; -- 1.8.4