Hi Thomas, Base on the previous conversation, at least it requires v2 to reword some comments.
> > >> 2. I propose to add addition comments on rte_epoll_wait() API > > >> description. For any signal, it causes an error return, user needs > > >> to handle. > > > Agreed. In addition, one conversion is not close. > > >> the default termination handler > > > I am not so experienced with this "default termination handler". Can > someone > > > clarify what it is so I could comment better about it? > > For example, you're handling SIGINT. After finishing your necessary app > > cleanup, then 'signal(SIGINT, SIG_DFL); raise(SIGINT);'. > > The default signal handler can terminate the interrupt thread. > > >> 1. Can you explain and add patch comments why default signal handler > > >> is not good enough to terminate app. > > > Yes if someone call tell me more about what it is so I can check it. Thanks, Cunming > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Saturday, July 09, 2016 1:36 AM > To: Liang, Cunming <cunming.liang at intel.com> > Cc: Matthew Hall <mhall at mhcomputing.net>; dev at dpdk.org > Subject: Re: [dpdk-dev,1/3] rte_interrupts: add rte_eal_intr_exit to shut down > IRQ thread > > Cunming, what is the status of this patchset, please? > > 2016-03-23 11:24, Liang, Cunming: > > Hi Mattew, > > > > Thank you for your time. > > > > On 3/22/2016 3:39 PM, Matthew Hall wrote: > > > On Mon, Mar 21, 2016 at 03:58:44PM +0800, Liang, Cunming wrote: > > >> the default termination handler > > > I am not so experienced with this "default termination handler". Can > someone > > > clarify what it is so I could comment better about it? > > For example, you're handling SIGINT. After finishing your necessary app > > cleanup, then 'signal(SIGINT, SIG_DFL); raise(SIGINT);'. > > The default signal handler can terminate the interrupt thread. > > > > > > > >> If EINTR is caused by some non-term purpose signals, are you going > > >> to exit the interrupt thread any way? > > > We should discuss what makes sense here. I'm just trying to get some > > > things > > > working and finding EINTR was getting eaten and causing infinite looping. > > SIGINT/SIGTERM causes EINTR return, while SIGUSR1 also can cause the > > EINTR return. For the dedicated EAL interrupt thread, it won't be > > expected to exit for all kinds of the cause. > > On this view, I'm in favor of your patch which cancel the interrupt > > thread, but don't directly return by the EINTR. > > > > > > > >> Without setting 'PTHREAD_CREATE_DETACHED' won't cause the infinite > > >> loop. However by using pthread_cancel to terminate the thread, > > >> indeed it's necessary to set 'PTHREAD_CREATE_DETACHED'. > > > My general understanding is that PTHREAD_CREATE_DETACHED should be > used for > > > any thread, which should not keep a process open by itself if it is > > > executing, > > > i.e. a "daemon thread". I believe the interrupt thread qualifies as such a > > > thread if I have understood everything right (which is hard to promise > > > when > > > you only work in DPDK in spare time). > > > > > >> It looks like 'pthread_cancel' is the right way and I saw it > > >> continue keeps current EINTR handling in EAL interrupt thread. > > > It is one option. Depending what makes the most sense. > > > > > >> 1. Can you explain and add patch comments why default signal handler > > >> is not good enough to terminate app. > > > Yes if someone call tell me more about what it is so I can check it. > > > > > >> 2. I propose to add addition comments on rte_epoll_wait() API > > >> description. For any signal, it causes an error return, user needs > > >> to handle. > > > Agreed. > > > > > >> 3. Will you do a favorite to add 'PTHREAD_CREATE_DETACHED' to all > > >> EAL pthread too. > > > As a spare time developer I am a bit conservative about too large of a > > > scope > > > and messing with code for other threads or features I didn't personally > > > use or > > > test. This is because I don't have the same QA resources as Intel / 6WIND > > > / > > > etc.. Some help from a full time developer would be great here. > > All right, reasonable to me. > > > > > > > >> Cunming > > > Matthew. > > >