From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id E6E9BB0B6 for ; Mon, 2 Jun 2014 23:39:56 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 02 Jun 2014 14:40:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.98,959,1392192000"; d="scan'208";a="541440298" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga001.fm.intel.com with ESMTP; 02 Jun 2014 14:40:05 -0700 Received: from irsmsx107.ger.corp.intel.com (163.33.3.99) by IRSMSX102.ger.corp.intel.com (163.33.3.155) with Microsoft SMTP Server (TLS) id 14.3.123.3; Mon, 2 Jun 2014 22:40:05 +0100 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.58]) by IRSMSX107.ger.corp.intel.com ([169.254.10.208]) with mapi id 14.03.0123.003; Mon, 2 Jun 2014 22:40:04 +0100 From: "Richardson, Bruce" To: Neil Horman Thread-Topic: [dpdk-dev] [PATCH v2 2/5] distributor: new packet distributor library Thread-Index: AQHPeyaGPXLCfjEU5U6ySUAwMGZJ25tXgkCAgAbWZ0A= Date: Mon, 2 Jun 2014 21:40:04 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B01AA31639@IRSMSX103.ger.corp.intel.com> References: <1400580057-30155-1-git-send-email-bruce.richardson@intel.com> <1401358338-23455-3-git-send-email-bruce.richardson@intel.com> <20140529134823.GD25784@hmsreliant.think-freely.org> In-Reply-To: <20140529134823.GD25784@hmsreliant.think-freely.org> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 2/5] distributor: new packet distributor library 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: Mon, 02 Jun 2014 21:39:57 -0000 > -----Original Message----- > From: Neil Horman [mailto:nhorman@tuxdriver.com] > Sent: Thursday, May 29, 2014 6:48 AM > To: Richardson, Bruce > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 2/5] distributor: new packet distributo= r library >=20 > > + > > +/* flush the distributor, so that there are no outstanding packets in = flight or > > + * queued up. */ > Its not clear to me that this is a distributor only function. You modifi= ed the > comments to indicate that lcores can't preform double duty as both a work= er > and > a distributor, which is fine, but it implies that there is a clear distin= ction > between functions that are 'worker' functions and 'distributor' functions= . > While its for the most part clear-ish (workers call rte_distributor_get_p= kt and > rte_distibutor_return_pkt, distibutors calls rte_distributor_create/proce= ss. > This is in a grey area. the analogy I'm thinking of here are kernel work= queues. > Theres a specific workqueue thread that processes the workqueue, but any > process > can sync or flush the workqueue, leading me to think this process can be = called > by a worker lcore. I can update comments here further, but I was hoping the way things were ri= ght now was clear enough. In the header and C files, I have the functions e= xplicitly split up into distributor and worker function sets, with a big bl= ock of text in the header at the start of each section explaining the threa= ding use of the follow functions.=20 >=20 > > +int > > +rte_distributor_flush(struct rte_distributor *d) > > +{ > > + unsigned wkr, total_outstanding =3D 0; > > + unsigned flushed =3D 0; > > + unsigned ret_start =3D d->returns.start, > > + ret_count =3D d->returns.count; > > + > > + for (wkr =3D 0; wkr < d->num_workers; wkr++) > > + total_outstanding +=3D d->backlog[wkr].count + > > + !!(d->in_flight_tags[wkr]); > > + > > + wkr =3D 0; > > + while (flushed < total_outstanding) { > > + > > + if (d->in_flight_tags[wkr] !=3D 0 || d->backlog[wkr].count) { > > + const int64_t data =3D d->bufs[wkr].bufptr64; > > + uintptr_t oldbuf =3D 0; > > + > > + if (data & RTE_DISTRIB_GET_BUF) { > > + flushed +=3D (d->in_flight_tags[wkr] !=3D 0); > > + if (d->backlog[wkr].count) { > > + d->bufs[wkr].bufptr64 =3D > > + backlog_pop(&d- > >backlog[wkr]); > > + /* we need to mark something as being > > + * in-flight, but it doesn't matter what > > + * as we never check it except > > + * to check for non-zero. > > + */ > > + d->in_flight_tags[wkr] =3D 1; > > + } else { > > + d->bufs[wkr].bufptr64 =3D > > + > RTE_DISTRIB_GET_BUF; > > + d->in_flight_tags[wkr] =3D 0; > > + } > > + oldbuf =3D data >> RTE_DISTRIB_FLAG_BITS; > > + } else if (data & RTE_DISTRIB_RETURN_BUF) { > > + if (d->backlog[wkr].count =3D=3D 0 || > > + move_backlog(d, wkr) =3D=3D 0) { > > + /* only if we move backlog, > > + * process this packet */ > > + d->bufs[wkr].bufptr64 =3D 0; > > + oldbuf =3D data >> > RTE_DISTRIB_FLAG_BITS; > > + flushed++; > > + d->in_flight_tags[wkr] =3D 0; > > + } > > + } > > + > > + store_return(oldbuf, d, &ret_start, &ret_count); > > + } > > + > I know the comments for move_backlog say you use that function here rathe= r > than > what you do in distributor_process because you're tracking the flush coun= t here. > That said, if you instead recomputed the total_outstanding count on each = loop > iteration, and tested it for 0, I think you could just reduce the flush > operation to a looping call to rte_distributor_process. It would save yo= u > having to maintain the flush code and the move_backlog code separately, w= hich > would be a nice savings. Yes, agreed, I should have spotted that myself. I'll look to rework this as= soon as I can. >=20 > > + if (++wkr =3D=3D d->num_workers) > > + wkr =3D 0; > Nit: wkr =3D ++wkr % d->num_workers avoids the additional branch in your = loop >=20 a) branch should be easily predictable as the number of workers doesn't cha= nge. So long as branch doesn't mis-predict there should be little or no per= f penalty to having it.=20 b) The compare plus update can also be done branchless using a "cmov" instr= uction if we want branch free code. c) The modulus operator is very slow and takes far more cycles than a branc= h would do. If we could limit the number of workers to a power of two, then= an & operation would work nicely, but that is too big a restriction to hav= e. So, in short, I think I'll keep this snippet as-is. :-)