From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753087AbcEDL6v (ORCPT ); Wed, 4 May 2016 07:58:51 -0400 Received: from mx2.suse.de ([195.135.220.15]:50012 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753003AbcEDL6t (ORCPT ); Wed, 4 May 2016 07:58:49 -0400 Date: Wed, 4 May 2016 13:58:47 +0200 (CEST) From: Miroslav Benes To: Josh Poimboeuf cc: Jiri Kosina , jeyu@redhat.com, pmladek@suse.com, jslaby@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, huawei.libin@huawei.com, minfei.huang@yahoo.com Subject: Re: [RFC PATCH] livepatch: allow removal of a disabled patch In-Reply-To: <20160504033619.5osteklgal3ixcbo@treble> Message-ID: References: <1462190242-24731-1-git-send-email-mbenes@suse.cz> <20160503213709.g66xr624pwn7oguu@treble> <20160504023948.ttb3ko2wnicwruwy@treble> <20160504033619.5osteklgal3ixcbo@treble> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 3 May 2016, Josh Poimboeuf wrote: > On Tue, May 03, 2016 at 09:39:48PM -0500, Josh Poimboeuf wrote: > > On Wed, May 04, 2016 at 12:31:12AM +0200, Jiri Kosina wrote: > > > On Tue, 3 May 2016, Josh Poimboeuf wrote: > > > > > > > > 1. Do we really need a completion? If I am not missing something > > > > > kobject_del() always waits for sysfs callers to leave thanks to kernfs > > > > > active protection. > > > > > > > > What do you mean by "kernfs active protection"? I see that > > > > kernfs_remove() gets the kernfs_mutex lock, but I can't find anywhere > > > > that a write to a sysfs file uses that lock. > > > > > > > > I'm probably missing something... > > > > > > I don't want to speak on Miroslav's behalf, but I'm pretty sure that what > > > he has on mind is per-kernfs_node active refcounting kernfs does (see > > > kernfs_node->active, and especially it's usage in __kernfs_remove()). > > > > > > More specifically, execution of store() and show() sysfs callbacks is > > > guaranteed (by kernfs) to happen with that particular attribute's active > > > reference held for reading (and that makes it impossible for that > > > attribute to vanish prematurely). > > > > Thanks, that makes sense. > > > > So what exactly is the problem the completion is trying to solve? Is it > > to ensure that the kobject has been cleaned up before it returns to the > > caller, in case the user wants to call klp_register() again after > > unregistering? > > > > If so, that's quite an unusual use case which I think we should just > > consider unsupported. In fact, if you try to do it, kobject_init() > > complains loudly because kobj->state_initialized is still 1 because > > kobjects aren't meant to be reused like that. > > ... and now I realize the point is actually to prevent the caller from > freeing klp_patch before kobject_cleanup() runs. Exactly. Sorry I was so brief. > So yeah, it looks like we need the completion in case > CONFIG_DEBUG_KOBJECT_RELEASE is enabled. > > Or alternatively we could convert patch->kobj to be dynamically > allocated instead of embedded in klp_patch. But that wouldn't help, would it? Dynamic kobjects registers generic release function dynamic_kobj_release() and that's it. We're in the same situation. I have got a feeling that dynamic kobjects are only for trivial cases. Moreover we use container_of() several times in the code and that does not work with dynamically allocated kobjects. Anyway I am really confused now. When I read changelog of c817a67ecba7 ("kobject: delayed kobject release: help find buggy drivers") all makes perfect sense. But isn't our situation somewhat special, because we have refcounts completely under control? So we know that once we call kobject_put() we can let a patch go... I must be missing something. It does not make sense to introduce completion just to satisfy a feature which was introduced to debug general cases. Miroslav