Date: Sun, 18 Jan 2015 13:55:13 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r277199 - in head/sys: fs/devfs kern Message-ID: <54BBAD31.701@selasky.org> In-Reply-To: <20150118113150.GQ42409@kib.kiev.ua> References: <20150115033109.GM42409@kib.kiev.ua> <54B76F2B.8040106@selasky.org> <20150115093841.GX42409@kib.kiev.ua> <54B79B25.3070707@selasky.org> <20150115115123.GA42409@kib.kiev.ua> <54B7AF2F.3080802@selasky.org> <20150116080332.GE42409@kib.kiev.ua> <54B8D31B.9030805@selasky.org> <20150118105410.GN42409@kib.kiev.ua> <54BB942A.5070200@selasky.org> <20150118113150.GQ42409@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Konstantin,
On 01/18/15 12:31, Konstantin Belousov wrote:
> On Sun, Jan 18, 2015 at 12:08:26PM +0100, Hans Petter Selasky wrote:
>> Hi Konstantin,
>>
>> On 01/18/15 11:54, Konstantin Belousov wrote:
>>> On Fri, Jan 16, 2015 at 10:00:11AM +0100, Hans Petter Selasky wrote:
>>>> Hi Konstantin,
>>
>> What about events for "devd" that a new character devices is present in
>> the system or being destroyed for user-space applications?
> It is up to the driver to decide. If wanted, it could post the pair of
> destroy/create itself (e.g., if there is some state which require
> the userspace to reset), or it could decide not to. The later is
> reasonable if the destroy/create is believed to be caused by a glitch
> rather than the actual replug.
I would really prefer if that path is chosen, that kern_conf.c exposes
an API for this. Consider the following case:
You have a daemon that is started and stopped based upon "devd" DEVFS
create/destroy events. The daemon is started when the create event is
received. Then the kernel side calls destroy_dev(), because the
application still keeps the file handle opened, the destroy_dev() never
sends the destroy event, and the application then never receives the
destroy event --- another example of a totally invisible deadlock.
>>>
>>> I find it very strange that you need sleepless operation which can
>>> _both_ destroy and create cdev. At least one operation of creation or
My simple argument against sleepable operation inside devfs functions
which are used to register and unregister a character devices is simply
that it leads to the fact we cannot make an atomic shutdown of a kernel
application which involves devfs and other kernel subsystems. Let me
explain.
It is not enough that devfs works alone, it needs to work with together
with other kernel APIs aswell. Let's make up an example. A kernel module
is using three different APIs, lets say USB, the callout subsystem and
devfs.
Devfs is calling USB to start and stop USB transfers.
The callout subsystem is calling selwakeup() and waking up devfs.
The USB subsystem is calling selwakeup() and waking up devfs aswell.
The USB subsystem is also calling destroy_dev().
This is a very high-level architectural thought and I hope you get it:
How can you tear down a kernel application using three different
subsystems in the kernel _atomically_ without having lingering callbacks?
To make the shutdown sequence atomic, we might want to use a mutex. Now
iff destroy_dev() is sleepable we need to drop that mutex when
destroying the character device, and then woops - the shutdown sequence
is no longer atomic. That's the root of the problem, and please think
beyond devfs alone. The problem happens when devfs is used together with
other APIs and applications in the kernel.
>>> destruction must sleep, at least in the current devfs design. It could
>>> be made sleepless, by making VFS visible part even less connected to the
>>> cdev part, but this is not how it (can) currently work.
>>
>> I think it would be good practice that all resources needed for creating
>> a character device can be allocated prior to registration. That means we
>> first should allocate all resources needed, but not register as a single
>> function.
>>
>> For example:
>>
>> Once make_dev() has returned, we can get an "d_open" callback. But
>> "si_drv1/2" is always set by drivers _after_ that "make_dev()" has
>> returned! This causes a race we could simply avoid by splitting the
>> make_dev() like I suggest. Instead of putting more logic and code inside
>> the drivers to handle the race!
> I wanted to tunnel the si_drv values through the make_dev().
> I.e. there could appear yet another variant of make_dev() which takes
> the initialization parameters for the si_* driver vars.
>
> I just did not have time/motivation to do the pass.
I might be interested in giving you a hand there, but not right now.
>>
>>>>
>>>>>
>>>>> This would also close a window where /dev node is non-existent or operate
>>>>> erronously.
>>>>
>>>> I'm not saying I plan so, but I think "cdevs" at some point need to
>>>> understand mutexes and locks. That means like with other API's in the
>>>> kernel we can associate a lock with the "cdev", and this lock is then
>>>> used to ensure an atomic shutdown of the system in a non-blocking
>>>> fashion. In my past experience multithreaded APIs should be high level
>>>> implemented like this:
>>>>
>>>> NON-BLOCKING methods:
>>>> lock(); **
>>>> xxx_start();
>>>> xxx_stop();
>>>> unlock();
>>>>
>>>> BLOCKING methods:
>>>> setup(); // init
>>>> unsetup(); // drain
>>>>
>>>> Any callbacks should always be called locked **
>>>>
>>>> In devfs there was no non-blocking stop before I added the delist_dev()
>>>> function.
>>>>
>>>>>
>>>>> You do not understand my point.
>>>>>
>>>>> I object against imposing one additional global reference on all cdevs
>>>>> just to cope with the delist hack. See the patch at the end of the message.
>>>>
>>>> It's fine by me.
>>>>>
>>>>> WRT destroy_dev_sched_cb() calling delist_dev(), even after calling
>>>>> delist_dev(), the node still exists in the /dev. It is only removed
>>>>> after populate loop is run sometime later. dev_sched() KPI is inheritly
>>>>> racy, drivers must handle the races for other reasons.
>>>>
>>>> The populate loop is all running under the dev_lock() from what I can
>>>> see and make_dev() is also keeping the same lock when inserting and
>>>> removing new cdevs. The populate loop should always give a consistent
>>> No, populate loop is not run under dev_mtx.
>>
>> Are you sure:
> Yes, I am sure. I explained it in the next sentences which you break from
> the first one in the citation.
"devfs_populate()" is called exclusivly locked by "dm_lock" and doesn't
return until an atomic TAILQ_FOREACH() of the "devp_list" under
"dev_lock()" is consistent - right? So the VFS node tree will not be
inconsistent under "dm_lock" either - right? Or is there something which
I don't see?
void
devfs_populate(struct devfs_mount *dm)
{
unsigned gen;
sx_assert(&dm->dm_lock, SX_XLOCKED);
gen = devfs_generation;
if (dm->dm_generation == gen)
return;
>>> It would not be able to
>>> allocate memory, even in M_NOWAIT fashion. The dev_mtx is after the
>>> vm and vnode locks. Only iteration over the cdevp_list is protected by
>>> dev_mtx, which is dropped right after something which require an
>>> action, is found on the list. Populate() needs some way to avoid
>>> reading inconsistent data from the cdev layer, but there is not
>>> guarantee that we are always up to date.
> ^^^^^^^^^
Are you taking the sleepable "dm_lock" into account?
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54BBAD31.701>