Further copy_from_user() discussion.

Further copy_from_user() discussion.

[Vadim Lobanov]

Hi,

Interested by the recent discussions concerning the copy_from_user()
function, I browsed the 2.6.11.7 kernel source, and came up with a few
questions.

1. Is there any particular reason why __copy_from_user_ll() is currently
EXPORT_SYMBOL()ed for i386? At least none of the in-tree modules
currently seem to use it, and __copy_from_user() seems like what most
would want anyway. If __copy_from_user_ll() is unexported, it looks like
we can eliminate the BUG_ON() statement within it.

2. Would it be possible to eliminate the might_sleep() call in
copy_from_user()? It seems that, very soon after, the __copy_from_user()
macro does another might_sleep(), with very few instructions in between.
But there might be some trick here that I'm missing.

Please enlighten. :-)

-Vadim Lobanov

Re: Further copy_from_user() discussion.

[Catalin Marinas]

Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> 2. Would it be possible to eliminate the might_sleep() call in
> copy_from_user()? It seems that, very soon after, the __copy_from_user()
> macro does another might_sleep(), with very few instructions in between.
> But there might be some trick here that I'm missing.

might_sleep() is used for debugging the possible sleep while in an
atomic operation. I think it is safe to check this for all the calls
to copy_from_user(), no matter if the access is OK or not (memset
being used in the latter case). The same is for
__copy_from_user(). Anyway, if you don't enable
CONFIG_DEBUG_SPINLOCK_SLEEP, the might_sleep() macro is empty.

-- 
Catalin

Re: Further copy_from_user() discussion.

[Vadim Lobanov]

On Thu, 14 Apr 2005, Catalin Marinas wrote:

> Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> > 2. Would it be possible to eliminate the might_sleep() call in
> > copy_from_user()? It seems that, very soon after, the __copy_from_user()
> > macro does another might_sleep(), with very few instructions in between.
> > But there might be some trick here that I'm missing.
>
> might_sleep() is used for debugging the possible sleep while in an
> atomic operation. I think it is safe to check this for all the calls
> to copy_from_user(), no matter if the access is OK or not (memset
> being used in the latter case). The same is for
> __copy_from_user(). Anyway, if you don't enable
> CONFIG_DEBUG_SPINLOCK_SLEEP, the might_sleep() macro is empty.
>
> --
> Catalin
>

Thanks for the response.

I think I misspoke a bit in my email above. The intent was not to
eliminate all might_sleep() calls from the copy_from_user() code path;
but rather juggle the source around a bit so there is only one
might_sleep() call per each code path. Currently, in the default case,
it calls it twice.

By the way, is the following still true about might_sleep()?
http://kerneltrap.org/node/3440/10103

-Vadim Lobanov

Re: Further copy_from_user() discussion.

[Catalin Marinas]

Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> I think I misspoke a bit in my email above. The intent was not to
> eliminate all might_sleep() calls from the copy_from_user() code path;
> but rather juggle the source around a bit so there is only one
> might_sleep() call per each code path. Currently, in the default case,
> it calls it twice.
>
> By the way, is the following still true about might_sleep()?
> http://kerneltrap.org/node/3440/10103

With Ingo's realtime-preempt patch, might_sleep() expands to
might_resched(). The latter expands to cond_resched() only if
CONFIG_PREEMPT_VOLUNTARY is enabled (for CONFIG_PREEMPT_RT this is not
needed since the kernel is involuntarily preemptible). In this case it
might be useful to have might_sleep() only called before memset().

-- 
Catalin

Re: Further copy_from_user() discussion.

[Vadim Lobanov]

On Fri, 15 Apr 2005, Catalin Marinas wrote:

> Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> > I think I misspoke a bit in my email above. The intent was not to
> > eliminate all might_sleep() calls from the copy_from_user() code path;
> > but rather juggle the source around a bit so there is only one
> > might_sleep() call per each code path. Currently, in the default case,
> > it calls it twice.
> >
> > By the way, is the following still true about might_sleep()?
> > http://kerneltrap.org/node/3440/10103
>
> With Ingo's realtime-preempt patch, might_sleep() expands to
> might_resched(). The latter expands to cond_resched() only if
> CONFIG_PREEMPT_VOLUNTARY is enabled (for CONFIG_PREEMPT_RT this is not
> needed since the kernel is involuntarily preemptible). In this case it
> might be useful to have might_sleep() only called before memset().
>
> --
> Catalin
>

I agree that might_sleep() needs to be placed in the code judiciously...
just probably not so close together as it is now. :-) I can work this
out in a patch, _if_ people want me to roll a patch in the first place.

-Vadim Lobanov