[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LNX.2.00.1605041343570.22292@pobox.suse.cz>
Date: Wed, 4 May 2016 13:58:47 +0200 (CEST)
From: Miroslav Benes <mbenes@...e.cz>
To: Josh Poimboeuf <jpoimboe@...hat.com>
cc: Jiri Kosina <jikos@...nel.org>, jeyu@...hat.com, pmladek@...e.com,
jslaby@...e.cz, live-patching@...r.kernel.org,
linux-kernel@...r.kernel.org, huawei.libin@...wei.com,
minfei.huang@...oo.com
Subject: Re: [RFC PATCH] livepatch: allow removal of a disabled patch
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
Powered by blists - more mailing lists