From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gavin Hu (Arm Technology China)" Subject: Re: [PATCH v3 2/4] kni: fix kni fifo synchronization Date: Wed, 10 Oct 2018 10:06:28 +0000 Message-ID: References: <1537364560-4124-1-git-send-email-phil.yang@arm.com> <1538989906-8349-1-git-send-email-phil.yang@arm.com> <1538989906-8349-2-git-send-email-phil.yang@arm.com> <20181008145308.0fbc64de@xeon-e3> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "jerin.jacob@caviumnetworks.com" , Honnappa Nagarahalli , Ola Liljedahl , "ferruh.yigit@intel.com" To: "Phil Yang (Arm Technology China)" , Stephen Hemminger Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0070.outbound.protection.outlook.com [104.47.2.70]) by dpdk.org (Postfix) with ESMTP id 211C41B4C5 for ; Wed, 10 Oct 2018 12:06:30 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Phil Yang (Arm Technology China) > Sent: Wednesday, October 10, 2018 5:59 PM > To: Stephen Hemminger > Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm > Technology China) ; Honnappa Nagarahalli > ; Ola Liljedahl ; > ferruh.yigit@intel.com > Subject: RE: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo synchronization > > Hi Hemminger, > > > -----Original Message----- > > From: Stephen Hemminger > > Sent: Tuesday, October 9, 2018 5:53 AM > > To: Phil Yang (Arm Technology China) > > Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm > > Technology China) ; Honnappa Nagarahalli > > ; Ola Liljedahl ; > > ferruh.yigit@intel.com > > Subject: Re: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo > > synchronization > > > > On Mon, 8 Oct 2018 17:11:44 +0800 > > Phil Yang wrote: > > > > > diff --git a/lib/librte_kni/rte_kni_fifo.h > > > b/lib/librte_kni/rte_kni_fifo.h index ac26a8c..70ac14e 100644 > > > --- a/lib/librte_kni/rte_kni_fifo.h > > > +++ b/lib/librte_kni/rte_kni_fifo.h > > > @@ -28,8 +28,9 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void > > > **data, unsigned num) { > > > unsigned i =3D 0; > > > unsigned fifo_write =3D fifo->write; > > > -unsigned fifo_read =3D fifo->read; > > > unsigned new_write =3D fifo_write; > > > +rte_smp_rmb(); > > > +unsigned fifo_read =3D fifo->read; > > > > > > > The patch makes sense, but this function should be changed to match > > kernel code style. > > That means no declarations after initial block, and use 'unsigned int' > > rather than 'unsigned' > > > > Also. why is i initialized? Best practice now is to not do gratitious > > initialization since it defeats compiler checks for accidental use of > uninitialized variables. > > > > What makes sense is something like: > > > > kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num) { > > unsigned int i, fifo_read, fifo_write, new_write; > > > > fifo_write =3D fifo->write; > > new_write =3D fifo_write; > > rte_smb_rmb(); > > fifo_read =3D fifo->read; > > > > Sorry, blaming you for issues which are inherited from original KNI cod= e. > > Maybe someone should run kernel checkpatch (not DPDK checkpatch) on it > > and fix those. > > Thanks for your comment. > > I think I can submit a new separate patch to fix this historical issue. > > Thanks, > Phil Yang I advised a separate patch to make this patch to the point and clean. -Gavin IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease notify the sender immediately and do not disclose the contents to any= other person, use it for any purpose, or store or copy the information in = any medium. Thank you.