From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 9E13BCB46 for ; Thu, 16 Jun 2016 18:42:04 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 16 Jun 2016 09:41:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,481,1459839600"; d="scan'208";a="1003397756" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by fmsmga002.fm.intel.com with ESMTP; 16 Jun 2016 09:41:24 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.183]) by IRSMSX151.ger.corp.intel.com ([169.254.4.151]) with mapi id 14.03.0248.002; Thu, 16 Jun 2016 17:41:22 +0100 From: "Iremonger, Bernard" To: Thomas Monjalon , "Richardson, Bruce" CC: "dev@dpdk.org" , "Doherty, Declan" , "Ananyev, Konstantin" , "Mcnamara, John" Thread-Topic: [dpdk-dev] [PATCH v3 3/4] bonding: take queue spinlock in rx/tx burst functions Thread-Index: AQHRxM18/A1UDGyp2k6AoPL2Wnlv+p/nDpiAgAAtvzCABOEFAIAAB/qAgAAnYdA= Date: Thu, 16 Jun 2016 16:41:21 +0000 Message-ID: <8CEF83825BEC744B83065625E567D7C21A03A6D7@IRSMSX108.ger.corp.intel.com> References: <1464280727-25752-2-git-send-email-bernard.iremonger@intel.com> <8CEF83825BEC744B83065625E567D7C21A038B2E@IRSMSX108.ger.corp.intel.com> <20160616143210.GB14984@bricha3-MOBL3> <5839452.IzJ3v2K0cK@xps13> In-Reply-To: <5839452.IzJ3v2K0cK@xps13> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOGI1YjJjYzEtODJjNi00YTlkLWI5MWItZGQ0YTAxNDM4MjVhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkxSN1J4bGpmdG16V202NzdPaW5wMmU2OUx1Ulo4dUNKVlNvQk1kNjVxc0k9In0= x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 3/4] bonding: take queue spinlock in rx/tx burst functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Jun 2016 16:42:05 -0000 Hi Thomas, > 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 b= e 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 ru= nning > 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. >=20 > 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. >=20 > So I would say NACK for 16.07 and maybe postpone to 16.11. Looking at the documentation for the bonding PMD. http://dpdk.org/doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.html In section 10.2 it states the following: Bonded devices support the dynamical addition and removal of slave devices = using the rte_eth_bond_slave_add / rte_eth_bond_slave_remove APIs. If a slave device is added or removed while traffic is running, there is th= e possibility of a segmentation fault in the rx/tx burst functions. This is= most likely to occur in the round robin bonding mode. This patch set fixes what appears to be a bug in the bonding PMD. Performance measurements have been made with this patch set applied and wit= hout the patches applied using 64 byte packets.=20 With the patches applied the following drop in performance was observed: % drop for fwd+io: 0.16% % drop for fwd+mac: 0.39% This patch set has been reviewed and ack'ed, so I think it should be applie= d in 16.07 Regards, Bernard.