Re: [PATCH] sysctl: add support for poll()

[Kay Sievers]

On Thu, Jun 2, 2011 at 15:56, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
> Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> writes:

>> With this patch in, if anyone wants to manage a file under /proc/sys
>> there's really a small amount of code to write. He only has to define
>> the new poll struct for that file.
>
> The support currently appears cumbersome to add, and it adds what appear
> to be unnecessary wake ups (say when the hostname in another uts
> namespace changes).

Yeah, *possibly* waking up once a day compared to *unconditionally*
waking up every second in every namespace and check if something has
changed. If that *possibly* should be optimized, which I think isn't
necessary at all, I guess the logic could be moved down to the
namespaced data.

> There is no explanation at all of why you care about the nis domainname.

Just the same reason as the hostname, we need to know when the
kernel's internal state changes. regardless who did it and why, system
services need to follow it.

> Since there does not appear to be a specific problem that this problem
> is being aimed at, since the code just looks like extra maintenance and
> since the code needed to support this appears to be unnecessarily
> cumbersome I am going to nack the patch for now.
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>

Please provide solid technical reasons, why we can't to the same we do
everywhere else since years, or provide working alternatives. Until
then:

Acked-By: Kay Sievers <kay.sievers@xxxxxxxx>

> If the goal here is just to fix the general case then we probably
> want to get inotify going on proc files instead of poll. ÂEither
> that or we want pollable files to appear as something besides files
> so that it is clear to their users that poll will work.

I think poll() is the natural interface if you care about the data in
a file. It's the same an single fd you care, and not some iniotify
watch, fd, pathname to register, and filter for whatever comes out
there.

We already use exactly the same semantics for quite some years for
/proc/mounts, /proc/swaps, /proc/mdstat, ... and all over /sys. The
patch just provides the missing pieces that /proc provides, but
/proc/sys is missing.

I don't disagree, it might be nice to have generic inotify support on
/proc for this or other problems. But unless you want to work on it
now, and it would solve these problems, I don't see how we can get the
functionality we need, and that seems to solved with this patch.
Besides the fact that I think poll() is the simple and better
solution.

Thanks,
Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/