Re: [dpdk-dev] [PATCH v2 2/5] distributor: new packet distributor library

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Jun 03, 2014 at 02:33:16PM +0000, Richardson, Bruce wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Tuesday, June 03, 2014 4:01 AM
> > To: Richardson, Bruce
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 2/5] distributor: new packet distributor library
> > 
> > On Mon, Jun 02, 2014 at 09:40:04PM +0000, Richardson, Bruce wrote:
> > >
> > >
> > > > -----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 distributor
> > library
> > > >
> > > > > +
> > > > > +/* 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.
> > >
> > > I can update comments here further, but I was hoping the way things were
> > right now was clear enough. In the header and C files, I have the functions
> > explicitly split up into distributor and worker function sets, with a big block of
> > text in the header at the start of each section explaining the threading use of the
> > follow functions.
> > >
> > Very well, we can let use be the determinant here.  We can leave it as is, and
> > if reports of lockups come in, we can change it, otherwise no harm done.
> > 
> Since I'm not a big fan of the "let's wait for the lock-ups" approach, I'll add in a single-line addition to each function's doxygen comment that should make its way into the official API docs. :-)
> 
If you're planning on collapsing the flush routine into an iterative call to
distributor_process anyway, then, sure, I'd appreciate it.  

Thanks!
Neil