From: Thomas Monjalon <thomas.monjalon@6wind.com>
To: Bruce Richardson <bruce.richardson@intel.com>,
"Iremonger, Bernard" <bernard.iremonger@intel.com>
Cc: dev@dpdk.org, "Doherty, Declan" <declan.doherty@intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 3/4] bonding: take queue spinlock in rx/tx burst functions
Date: Thu, 16 Jun 2016 17:00:43 +0200 [thread overview]
Message-ID: <5839452.IzJ3v2K0cK@xps13> (raw)
In-Reply-To: <20160616143210.GB14984@bricha3-MOBL3>
2016-06-16 15:32, Bruce Richardson:
> On Mon, Jun 13, 2016 at 01:28:08PM +0100, Iremonger, Bernard wrote:
> > > Why does this particular PMD need spinlocks when doing RX and TX, while
> > > other device types do not? How is adding/removing devices from a bonded
> > > device different to other control operations that can be done on physical
> > > PMDs? Is this not similar to say bringing down or hotplugging out a physical
> > > port just before an RX or TX operation takes place?
> > > For all other PMDs we rely on the app to synchronise control and data plane
> > > operation - why not here?
> > >
> > > /Bruce
> >
> > This issue arose during VM live migration testing.
> > For VM live migration it is necessary (while traffic is running) to be able to remove a bonded slave device, stop it, close it and detach it.
> > It a slave device is removed from a bonded device while traffic is running a segmentation fault may occur in the rx/tx burst function. The spinlock has been added to prevent this occurring.
> >
> > The bonding device already uses a spinlock to synchronise between the add and remove functionality and the slave_link_status_change_monitor code.
> >
> > Previously testpmd did not allow, stop, close or detach of PMD while traffic was running. Testpmd has been modified with the following patchset
> >
> > http://dpdk.org/dev/patchwork/patch/13472/
> >
> > It now allows stop, close and detach of a PMD provided in it is not forwarding and is not a slave of bonded PMD.
> >
> I will admit to not being fully convinced, but if nobody else has any serious
> objections, and since this patch has been reviewed and acked, I'm ok to merge it
> in. I'll do so shortly.
Please hold on.
Seeing locks introduced in the Rx/Tx path is an alert.
We clearly need a design document to explain where locks can be used
and what are the responsibility of the control plane.
If everybody agrees in this document that DPDK can have some locks
in the fast path, then OK to merge it.
So I would say NACK for 16.07 and maybe postpone to 16.11.
next prev parent reply other threads:[~2016-06-16 15:00 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-05 15:14 [dpdk-dev] [PATCH 0/5] bonding: locks Bernard Iremonger
2016-05-05 15:14 ` [dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock Bernard Iremonger
2016-05-05 17:12 ` Stephen Hemminger
2016-05-06 10:32 ` Declan Doherty
2016-05-06 15:55 ` Stephen Hemminger
2016-05-13 17:10 ` Ananyev, Konstantin
2016-05-13 17:18 ` Ananyev, Konstantin
2016-05-26 16:24 ` Iremonger, Bernard
2016-05-05 15:14 ` [dpdk-dev] [PATCH 2/5] bonding: add read/write lock to rx/tx burst functions Bernard Iremonger
2016-05-05 15:14 ` [dpdk-dev] [PATCH 3/5] bonding: remove memcopy of slaves from rx/tx burst function Bernard Iremonger
2016-05-05 15:14 ` [dpdk-dev] [PATCH 4/5] bonding: add read/write lock to stop function Bernard Iremonger
2016-05-05 15:15 ` [dpdk-dev] [PATCH 5/5] bonding: add read/write lock to the link_update function Bernard Iremonger
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 0/6] bonding: locks Bernard Iremonger
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 1/6] bonding: add spinlock to rx and tx queues Bernard Iremonger
2016-06-10 18:12 ` Ananyev, Konstantin
2016-06-12 17:11 ` [dpdk-dev] [PATCH v3 0/4] bonding: locks Bernard Iremonger
2016-06-12 17:11 ` [dpdk-dev] [PATCH v3 1/4] bonding: add spinlock to rx and tx queues Bernard Iremonger
2016-06-12 17:11 ` [dpdk-dev] [PATCH v3 2/4] bonding: grab queue spinlocks in slave add and remove Bernard Iremonger
2016-06-12 17:11 ` [dpdk-dev] [PATCH v3 3/4] bonding: take queue spinlock in rx/tx burst functions Bernard Iremonger
2016-06-13 9:18 ` Bruce Richardson
2016-06-13 12:28 ` Iremonger, Bernard
2016-06-16 14:32 ` Bruce Richardson
2016-06-16 15:00 ` Thomas Monjalon [this message]
2016-06-16 16:41 ` Iremonger, Bernard
2016-06-16 18:38 ` Thomas Monjalon
2017-02-15 18:01 ` Ferruh Yigit
2017-02-16 9:13 ` Bruce Richardson
2017-02-16 11:39 ` Iremonger, Bernard
2017-02-20 11:15 ` Ferruh Yigit
2016-09-09 11:29 ` Ferruh Yigit
2016-06-12 17:11 ` [dpdk-dev] [PATCH v3 4/4] bonding: remove memcpy from " Bernard Iremonger
2016-09-11 12:39 ` Yuanhan Liu
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 2/6] bonding: grab queue spinlocks in slave add and remove Bernard Iremonger
2016-06-10 18:14 ` Ananyev, Konstantin
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 3/6] bonding: take queue spinlock in rx/tx burst functions Bernard Iremonger
2016-06-10 18:14 ` Ananyev, Konstantin
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 4/6] bonding: add spinlock to stop function Bernard Iremonger
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 5/6] bonding: add spinlock to link update function Bernard Iremonger
2016-05-26 16:38 ` [dpdk-dev] [PATCH v2 6/6] bonding: remove memcpy from burst functions Bernard Iremonger
2016-06-10 18:15 ` Ananyev, Konstantin
2016-06-10 14:45 ` [dpdk-dev] [PATCH v2 0/6] bonding: locks Bruce Richardson
2016-06-10 18:24 ` Iremonger, Bernard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5839452.IzJ3v2K0cK@xps13 \
--to=thomas.monjalon@6wind.com \
--cc=bernard.iremonger@intel.com \
--cc=bruce.richardson@intel.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).