Re: [PATCH] mm: get_user_pages() stores ERR_PTR() in pages[i] on failure

[Leon Woestenberg <leon.woestenberg@gmail.com>]

Hello,

thanks for looking along on this one.

On Tue, Aug 4, 2009 at 2:00 PM, Hugh Dickins<hugh.dickins@tiscali.co.uk> wrote:
> On Tue, 4 Aug 2009, Brice Goglin wrote:
>>
>> >> I wonder if we should change get_user_pages to store ERR_PTR(ret)
>> >> in page[i] when it fails to get page #i.
>> >>
>> > Yes, I would see that as an improvement in finding out why rc <
>> > nr_pages, in case rc > 0.
>> >
>> > Also I think it does not break existing users.
>> >
> ... some callers of get_user_pages() might expect it not to touch
> entries in the page array beyond the fail point: for example, they
> might start off with a zeroed array, then when they come to put_page
> on the array afterwards, would skip any NULL entries found - without
> having to have noted the returned number of pages.  I don't think
> that way of working is supported by any guarantee in documentation,
> but it's not unreasonable to expect it to behave in that way.
>
Thanks for checking this -- obviously me thinking it would not break
existing users is false.

> Of course we _could_ fix individual callers to work with it, but
> for me they sound a warning that it's too late to change now.
>
The alternative seems to be to describe a best practise of calling
get_user_pages() on the failing page to find out why it failed.
(and hoping it does not return rc > 0 on the second call.)

A better function API would be to return the number of succesfully
gotten pages in a pointer
rc = get_user_pages2(, nr_pages ,..., &nr_pages_gotten);
where rc ==0 iff (nr_pages == nr_pages_gotten).


BTW, in my case I'm hitting the (ret & VM_FAULT_SIGBUS) case. I
suspect I have a multithreading concurrency race, as I have DMA in
both directions to a ring buffer set of files. Seems one over runs the
other, thus the file gets mapped in with different protection, as well
as conflicting DMA directions.

-EBADUSER.
-- 
Leon