From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id C05D2312 for ; Thu, 29 May 2014 15:48:18 +0200 (CEST) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Wq0gy-00031V-AK; Thu, 29 May 2014 09:48:29 -0400 Date: Thu, 29 May 2014 09:48:23 -0400 From: Neil Horman To: Bruce Richardson Message-ID: <20140529134823.GD25784@hmsreliant.think-freely.org> References: <1400580057-30155-1-git-send-email-bruce.richardson@intel.com> <1401358338-23455-3-git-send-email-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1401358338-23455-3-git-send-email-bruce.richardson@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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: Thu, 29 May 2014 13:48:19 -0000 > + > +/* 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 modified the comments to indicate that lcores can't preform double duty as both a worker and a distributor, which is fine, but it implies that there is a clear distinction between functions that are 'worker' functions and 'distributor' functions. While its for the most part clear-ish (workers call rte_distributor_get_pkt and rte_distibutor_return_pkt, distibutors calls rte_distributor_create/process. This is in a grey area. the analogy I'm thinking of here are kernel workqueues. 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. > +int > +rte_distributor_flush(struct rte_distributor *d) > +{ > + unsigned wkr, total_outstanding = 0; > + unsigned flushed = 0; > + unsigned ret_start = d->returns.start, > + ret_count = d->returns.count; > + > + for (wkr = 0; wkr < d->num_workers; wkr++) > + total_outstanding += d->backlog[wkr].count + > + !!(d->in_flight_tags[wkr]); > + > + wkr = 0; > + while (flushed < total_outstanding) { > + > + if (d->in_flight_tags[wkr] != 0 || d->backlog[wkr].count) { > + const int64_t data = d->bufs[wkr].bufptr64; > + uintptr_t oldbuf = 0; > + > + if (data & RTE_DISTRIB_GET_BUF) { > + flushed += (d->in_flight_tags[wkr] != 0); > + if (d->backlog[wkr].count) { > + d->bufs[wkr].bufptr64 = > + 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] = 1; > + } else { > + d->bufs[wkr].bufptr64 = > + RTE_DISTRIB_GET_BUF; > + d->in_flight_tags[wkr] = 0; > + } > + oldbuf = data >> RTE_DISTRIB_FLAG_BITS; > + } else if (data & RTE_DISTRIB_RETURN_BUF) { > + if (d->backlog[wkr].count == 0 || > + move_backlog(d, wkr) == 0) { > + /* only if we move backlog, > + * process this packet */ > + d->bufs[wkr].bufptr64 = 0; > + oldbuf = data >> RTE_DISTRIB_FLAG_BITS; > + flushed++; > + d->in_flight_tags[wkr] = 0; > + } > + } > + > + store_return(oldbuf, d, &ret_start, &ret_count); > + } > + I know the comments for move_backlog say you use that function here rather than what you do in distributor_process because you're tracking the flush count 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 you having to maintain the flush code and the move_backlog code separately, which would be a nice savings. > + if (++wkr == d->num_workers) > + wkr = 0; Nit: wkr = ++wkr % d->num_workers avoids the additional branch in your loop Regards Neil