Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753838AbbKXNaX (ORCPT ); Tue, 24 Nov 2015 08:30:23 -0500 Received: from mail-ob0-f176.google.com ([209.85.214.176]:36165 "EHLO mail-ob0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753387AbbKXNaU (ORCPT ); Tue, 24 Nov 2015 08:30:20 -0500 Message-ID: <56546669.30006@acm.org> Date: Tue, 24 Nov 2015 07:30:17 -0600 From: Corey Minyard Reply-To: minyard@acm.org User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Petr Mladek CC: Andrew Morton , Oleg Nesterov , Tejun Heo , Ingo Molnar , Peter Zijlstra , Steven Rostedt , "Paul E. McKenney" , Josh Triplett , Thomas Gleixner , Linus Torvalds , Jiri Kosina , Borislav Petkov , Michal Hocko , linux-mm@kvack.org, Vlastimil Babka , linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, openipmi-developer@lists.sourceforge.net Subject: Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API References: <1447853127-3461-1-git-send-email-pmladek@suse.com> <1447853127-3461-18-git-send-email-pmladek@suse.com> <56536AA6.5040102@acm.org> <20151124121233.GH10750@pathway.suse.cz> In-Reply-To: <20151124121233.GH10750@pathway.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3963 Lines: 120 On 11/24/2015 06:12 AM, Petr Mladek wrote: > On Mon 2015-11-23 13:36:06, Corey Minyard wrote: >> >> On 11/18/2015 07:25 AM, Petr Mladek wrote: >>> Kthreads are currently implemented as an infinite loop. Each >>> has its own variant of checks for terminating, freezing, >>> awakening. In many cases it is unclear to say in which state >>> it is and sometimes it is done a wrong way. >>> >>> The plan is to convert kthreads into kthread_worker or workqueues >>> API. It allows to split the functionality into separate operations. >>> It helps to make a better structure. Also it defines a clean state >>> where no locks are taken, IRQs blocked, the kthread might sleep >>> or even be safely migrated. >>> >>> The kthread worker API is useful when we want to have a dedicated >>> single thread for the work. It helps to make sure that it is >>> available when needed. Also it allows a better control, e.g. >>> define a scheduling priority. >>> >>> This patch converts kipmi kthread into the kthread worker API because >>> it modifies the scheduling priority. The change is quite straightforward. >> I think this is correct. That code was hard to get right, but I don't >> see where any >> logic is actually changed. > I believe that it was hard to make it working. > > >> This also doesn't really look any simpler (you end up with more LOC than >> you did before :) ), >> though it will make things more consistent and reduce errors and that's >> a good thing. > I have just realized that the original code actually looks racy. For > example, it does: > > __set_current_state(TASK_INTERRUPTIBLE); > schedule(); > > without rechecking the state in between. There might already be a new > message and it might miss the wake_up_process(). Similar problem is > with the schedule_timeout_interruptible(100); I mean: > > > CPU 0 CPU 1 > > > ipmi_thread() > spin_lock_irqsave(); > smi_result = smi_event_handler(); > spin_unlock_irqrestore(); > > [...] > else if (smi_result == SI_SM_IDLE) > /* true */ > if (atomic_read(need_watch)) { > /* true */ > > sender() > spin_lock_irqsave() > check_start_timer_thread() > wake_up_process() > > /* > * NOPE because kthread > * is not sleeping > */ > > schedule_timeout_interruptible(100); > > /* > * We sleep 100 jiffies but > * there is a pending message. > */ Yes, I knew the code was racy, but this is a performance optimization and it wasn't that important to get it perfect. The thread wouldn't actually wait 100 jiffies, it would just be run by timer interrupts for that time. > > This is not a problem with the kthread worker API because > > mod_delayed_kthread_work(smi_info->worker, > &smi_info->work, 0); > > would queue the work to be done immediately and > > queue_delayed_kthread_work(smi_info->worker, > &smi_info->work, 100); > > would do nothing in this case. And indeed this is a lot better. > >> My only comment is I would like the worker function named ipmi_worker, >> not ipmi_func. > You probably want it because the original name was ipmi_thread. But > it might cause confusion with new_smi->worker. The function gets > assigned to work->func, see struct kthread_work. Therefore I think that > _func suffix makes more sense. My problem with _func is that it's way too generic. Is this a function that handled IPMI messages? Message done handling? I'm not enamored with my name, but I want something that gives a better indication of what the function does. ipmi_kthread_worker_func() would be fine with me. Thanks, -corey >> Reviewed-by: Corey Minyard > > Thanks a lot for review, > Petr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/