From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752244AbaHRIwM (ORCPT ); Mon, 18 Aug 2014 04:52:12 -0400 Received: from cantor2.suse.de ([195.135.220.15]:54256 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752170AbaHRIwI (ORCPT ); Mon, 18 Aug 2014 04:52:08 -0400 Date: Mon, 18 Aug 2014 10:52:03 +0200 Message-ID: From: Takashi Iwai To: Oleg Nesterov Cc: "Luis R. Rodriguez" , "Luis R. Rodriguez" , gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, Tetsuo Handa , Joseph Salisbury , Kay Sievers , One Thousand Gnomes , Tim Gardner , Pierre Fersing , Andrew Morton , Benjamin Poirier , Nagalakshmi Nandigama , Praveen Krishnamoorthy , Sreekanth Reddy , Abhijit Mahajan , Hariprasad S , Santosh Rastapur , MPT-FusionLinux.pdl@avagotech.com, linux-scsi@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit() In-Reply-To: <20140817182138.GA4411@redhat.com> References: <1407882507-325-2-git-send-email-mcgrof@do-not-panic.com> <20140813175101.GA7156@redhat.com> <20140814231028.GQ21930@wotan.suse.de> <20140815143902.GB13222@redhat.com> <20140816025007.GB3347@wotan.suse.de> <20140817122527.GA30546@redhat.com> <20140817124803.GA31996@redhat.com> <20140817125505.GA32429@redhat.com> <20140817174624.GE3347@wotan.suse.de> <20140817182138.GA4411@redhat.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Sun, 17 Aug 2014 20:21:38 +0200, Oleg Nesterov wrote: > > On 08/17, Luis R. Rodriguez wrote: > > > > In the last iteration that I have stress tested for corner cases I just > > get_task_struct() on the init and then put_task_struct() at the exit, is that > > fine too or are there reasons to prefer the module stuff? > > I am fine either way. > > I like the Takashi's idea because if sys_delete_module() is called before > initfn() completes it will return -EBUSY and not hang in TASK_UNINTERRUPTIBLE > state. But this is not necessarily good, so I leave this to you and Takashi. Another merit of fiddling with module count is that the thread object isn't referred in other than module_init. That is, we'd need only module_init() implementation like below (thanks to Oleg's advice): #define module_long_probe_init(initfn) \ static int _long_probe_##initfn(void *arg) \ { \ module_put_and_exit(initfn()); \ return 0; \ } \ static int __init __long_probe_##initfn(void) \ { \ struct task_struct *__init_thread = \ kthread_create(_long_probe_##initfn, \ NULL, #initfn); \ if (IS_ERR(__init_thread)) \ return PTR_ERR(__init_thread); \ __module_get(THIS_MODULE); \ wake_up_process(__init_thread); \ return 0; \ } \ module_init(__long_probe_##initfn) ... and module_exit() remains identical as the normal version. But, it's really a small difference, and I don't mind much which way to take, too. > > +/* > > + * Linux device drivers must strive to handle driver initialization > > + * within less than 30 seconds, > > Well, perhaps the comment should name the reason ;) > > > if device probing takes longer > > + * for whatever reason asynchronous probing of devices / loading > > + * firmware should be used. If a driver takes longer than 30 second > > + * on the initialization path > > Or if the initialization code can't handle the errors properly (say, > mptsas can't handle the errors caused by SIGKILL). > > > + * Drivers that use this helper should be considered broken and in need > > + * of some serious love. > > + */ > > Yes. > > > +#define module_long_probe_init(initfn) \ > > + static struct task_struct *__init_thread; \ > > + static int _long_probe_##initfn(void *arg) \ > > + { \ > > + return initfn(); \ > > + } \ > > + static inline __init int __long_probe_##initfn(void) \ > > + { \ > > + __init_thread = kthread_create(_long_probe_##initfn,\ > > + NULL, \ > > + #initfn); \ > > + if (IS_ERR(__init_thread)) \ > > + return PTR_ERR(__init_thread); \ > > + /* \ > > + * callback won't check kthread_should_stop() \ > > + * before bailing, so we need to protect it \ > > + * before running it. \ > > + */ \ > > + get_task_struct(__init_thread); \ > > + wake_up_process(__init_thread); \ > > + return 0; \ > > + } \ > > + module_init(__long_probe_##initfn); > > + > > +/* To be used by modules that require module_long_probe_init() */ > > +#define module_long_probe_exit(exitfn) \ > > + static inline void __long_probe_##exitfn(void) \ > > + { \ > > + int err; \ > > + /* \ > > + * exitfn() will not be run if the driver's \ > > + * real probe which is run on the kthread \ > > + * failed for whatever reason, this will \ > > + * wait for it to end. \ > > + */ \ > > + err = kthread_stop(__init_thread); \ > > + if (!err) \ > > + exitfn(); \ > > + put_task_struct(__init_thread); \ > > + } \ > > + module_exit(__long_probe_##exitfn); > > Both inline's look misleading, gcc will generate the code out-of-line > anyway. But this is cosmetic. And for cosmetic reasons, since the 1st > macro uses __init, the 2nd one should probably use __exit. Yes, and it'd be better to mention not to mark initfn with __init prefix. (Meanwhile exitfn can be with __exit prefix.) thanks, Takashi