Re: [dpdk-dev] [PATCH v3] distributor_app: new sample app

[Neil Horman <nhorman@tuxdriver.com>]

On Mon, Oct 06, 2014 at 05:34:13PM +0000, Pattan, Reshma wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Monday, October 6, 2014 3:45 PM
> > To: Pattan, Reshma
> > Cc: dev@dpdk.org; Richardson, Bruce
> > Subject: Re: [dpdk-dev] [PATCH v3] distributor_app: new sample app
> > 
> > On Mon, Oct 06, 2014 at 02:16:22PM +0000, Pattan, Reshma wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > Sent: Wednesday, October 1, 2014 5:08 PM
> > > > To: Richardson, Bruce
> > > > Cc: Pattan, Reshma; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v3] distributor_app: new sample app
> > > >
> > > > > > >
> > > > > > > 1)I had sent v5 patch which handles graceful shutdown of rx
> > > > > > > and tx threads upon SIGINT
> > > > > > I see it and will take a look shortly, thanks.
> > > > > >
> > > > > > > 2)Worker thread graceful shutdown was not handled as of now as
> > > > > > > it needs
> > > > some change in lcore_worker logic , which will be done in future
> > enhancements.
> > > > > > Not sure I understand what you mean here.  Can you elaborate?
> > > > > >
> > > rte_distributor_process which runs as part of rx thread will process incoming
> > packets and checks for any requests for the packets from worker threads .
> > > If request is seen, it adds the packet/work  to particular workers back log and
> > proceed with processing of  next packet.
> > >  If no request seen the packet index will not be incremented and the while loop
> > which is conditionally based on packet indexing  runs in a continuous loop
> > without breaking and rx thread will not proceed with next statement execution
> > until unless rte_distributor_process  comes out of while loop.
> > > This issue happens only when we enable graceful shutdown logic for both
> > rx/worker threads, as workers threads gets killed and no request seen by rx
> > thread and it stucks.
> > > Hence as of now graceful shutdown logic is provided only for rx thread. For
> > worker threads will check what can be done in next enhancements.
> > >
> > > Thanks,
> > > Reshma
> > >
> > 
> > I see what you're saying, Once you make a call to rte_distributor_get_pkt, you
> > have no way to gracefully shut down use of the rte_distributor_library. Not just
> > this application, but any application.  Thats just not sane, and suggests that we
> > integrated the rte_distributor library too soon.  I would suggest that you prefix
> > this patch with an update to the rte distributor library to allow
> > rte_distributor_get_pkt and friends to return NULL if the queue is emtpy.
> > Applications should be checking the return value for NULL anyway, and can
> > preform the rte_pause operation.  Then update this patch to do a clean exit.  To
> > say that "we will check what can be done in the next enhancements" is to say
> > that this won't be addressed again until a paying custmoer gripes about it.
> > Neil
> > 
> 
> Hi Neil,
> 
> We have rte_distributor_request_pkt and rte_distributor_poll_pkt() in dpdk.org,  which can be used together(in place of  rte_distributor_get_pkt)
>  to check empty queue condition I believe. So no separate changes needed.
> Though we replace to rte_distributor_get_pkt with above two, the issue will not be solved as the thread that gets blocked is rx thread but not worker thread.
> Rx thread gets blocked in rte_distributore_process due to non-availability of requests from worker. 
>  How about sending dummy request_pkts upon SIGINT and allow rte_distributore_process to get to completion? 
> 
I suppose creating a flagged dummy packet to ensure that the worker threads can
exit their spin loops works fine, yes.

Neil

> Thanks,
> Reshma
> 
> > > > > > > 3)Freeing of mempool is also not handled , as the framework
> > > > > > > support is not
> > > > available.
> > > > > > Ew, I hadn't noticed that, freeing of mempools seems like
> > > > > > something we should implement.
> > > > > >
> > > > > > > 4)Cleaning of rx/tx queues not done, as it needs some
> > > > > > > extensive logic
> > > > which we haven't planned as of now. Will check the possibility of doing it in
> > > > future  enhancements    i.e in next version of sample application.
> > > > > > We can't just flush the queues after we shutdown the workers?  I
> > > > > > presume a queue flush operation exists, yes?
> > > > > > Neil
> > > > >
> > > > > Other than code hygiene, which does have some value in itself, I
> > > > > can't really see what the practical point of such cleanup would be.
> > > > >
> > > > This is really the only assertion I'm trying to make.  I understand
> > > > this application won't suffer from exiting uncleanly, and that makes
> > > > the need for preforming cleanup little more than overhead.
> > > >
> > > > But that said, hygine is exactly the point I'm driving at here.
> > > > These are example applications, that presumably people look at when
> > > > writing their own apps.  If you don't do things properly, people
> > > > looking at your code are less likely to do them as well.  Even if it
> > > > doesn't hurt for you to exit uncleanly, it will hurt someone, and if
> > > > they look to these examples as a source of best practices, it seems
> > > > to me that it would be in everyones interest, if best practices were
> > demonstrated.
> > > >
> > > > Neil
> > >
> > > --------------------------------------------------------------
> > > Intel Shannon Limited
> > > Registered in Ireland
> > > Registered Office: Collinstown Industrial Park, Leixlip, County
> > > Kildare Registered Number: 308263 Business address: Dromore House,
> > > East Park, Shannon, Co. Clare
> > >
> > > This e-mail and any attachments may contain confidential material for the sole
> > use of the intended recipient(s). Any review or distribution by others is strictly
> > prohibited. If you are not the intended recipient, please contact the sender and
> > delete all copies.
> > >
> > >
> > >
> --------------------------------------------------------------
> Intel Shannon Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> Business address: Dromore House, East Park, Shannon, Co. Clare
> 
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> 
> 
>