[PATCH] Really start using the page walking API

[PATCH] Really start using the page walking API

[Ingo Oeser]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Hi all,

here come some improvements of the page walking API code.

First: make_pages_present() would do an infinite recursion, if
   used in find_extend_vma(). I fixed this. Might as well have
   caused the ntp crash, that has been observed. 
   So these make_pages_present parts are really important.

I also did the promised rewrite of make_pages_present() and its
users.

MM-Gurus: Please double check, that I always provide the right vma.

I also did two sample implementations (Kai and Doug, this is why
you are CC'ed) of the scatter list walking and removed ~100
lines of code while doing it.

Gerd: Your video-buf.[ch] is next on my list and I must coalesce 
   videobuf_pages_to_sg() and videobuf_init_user() to do it
   efficiently. May be you can come up with a better solution for
   this or are already working on sth. here. If not, I'll do it
   my way first and wait for your approval ;-)

That's all for now. I omitted the patches, to make 2.5.49-mm1
compile, as usual.

Patch against 2.5.49-mm1 attached.

diffstat of this patch:

 drivers/scsi/sg.c         |  102 +++++++++----------------------------------
 drivers/scsi/st.c         |  109 ++++++++--------------------------------------
 include/linux/mm.h        |    2 
 include/linux/page_walk.h |   13 +++--
 mm/mlock.c                |    2 
 mm/mmap.c                 |   11 ++--
 mm/mremap.c               |    4 -
 mm/page_walk.c            |   83 ++++++++++++++++++++++++-----------
 8 files changed, 119 insertions, 207 deletions

Regards

Ingo Oeser
-- 
Science is what we can tell a computer. Art is everything else. --- D.E.Knuth

[-- Attachment #2: page-walk-api-2.5.49-mm1-improvements.patch.bz2 --]
[-- Type: application/octet-stream, Size: 5070 bytes --]

Re: [PATCH] Really start using the page walking API

[Ingo Oeser]

Hi Andrew,
hi lkmm,

On Sun, Nov 24, 2002 at 11:34:49PM +0100, Ingo Oeser wrote:
> First: make_pages_present() would do an infinite recursion, if
>    used in find_extend_vma(). I fixed this. Might as well have
>    caused the ntp crash, that has been observed. 
>    So these make_pages_present parts are really important.

Ok, I looked deeper and saw, that the original code had the same
"problem", but it was always ensured, that this recursion is not
triggered.

find_extend_vma() returns right before calling make_pages_present(), if
that vma is not a growable stack. For the second call of
find_extend_vma() (in old get_user_pages() code) this vma has
already been successfully grown, so recursion is limited to one
level and it works by magic ;-)

My latest patch just made that more explicit, by passing the vma
directly from the make_pages_present caller down to the
walk_user_pages() and thus skipping the vma search.

If sth. goes wrong we still catch the BUG_ON() checks, so no harm
will be done to data.

In short: I made deep magic more visible here and reduced stack
   usage again. But I did NOT fix the ntp BUG, because it wasn't
   caused by this code.

Regards
-- 
Science is what we can tell a computer. Art is everything else. --- D.E.Knuth
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [PATCH] Really start using the page walking API

[Gerd Knorr]

On Sun, Nov 24, 2002 at 11:34:49PM +0100, Ingo Oeser wrote:
> Hi all,
> 
> here come some improvements of the page walking API code.

Is the API already in linus tree?

> Gerd: Your video-buf.[ch] is next on my list and I must coalesce 
>    videobuf_pages_to_sg() and videobuf_init_user() to do it
>    efficiently. May be you can come up with a better solution for
>    this or are already working on sth. here. If not, I'll do it
>    my way first and wait for your approval ;-)

Hmm.  I'm looking at the code, and it doesn't make much sense to do
it the current way (i.e. have page walking and scatter list building
splitted up).  As I'm not a vm expert I'm not sure what the right way to
handle this is.

The videobuf_init_*() functions are called when the video buffer is
setup, i.e. if some video4linux application calls mmap().

The videobuf_dma_pci_map() function (the one which calls
videobuf_pages_to_sg()) is called to prepare the video buffer for DMA,
i.e. if the application asked the driver to actually capture an video
frame into that buffer.


Right now it works basically that way, party for historical reasons
because the code used to use kiobufs:

videobuf_init_user():
	get_user_pages()

videobuf_dma_pci_map():
	lock pages for DMA	[videobuf_lock()]
	build scatter list	[videobuf_pages_to_sg()]
	pci_map_sg()


I think it can be cleaned up to work this way:

videobuf_init_user():
	walk pages & build scatter list (using the new API in 2.5 and
		the existing videobuf_* functions in 2.4).

videobuf_dma_pci_map():
	lock pages for DMA
	pci_map_sg()

Will this work correctly?  Ou should the page walking better be done in
the videobuf_dma_pci_map() function?

A generic function which accepts a scatter list and locks the pages in
there for DMA would be nice btw.

  Gerd

-- 
You can't please everybody.  And usually if you _try_ to please
everybody, the end result is one big mess.
				-- Linus Torvalds, 2002-04-20
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [PATCH] Really start using the page walking API

[Andrew Morton]

Gerd Knorr wrote:
> 
> ...
> videobuf_dma_pci_map():
>         lock pages for DMA      [videobuf_lock()]
>         build scatter list      [videobuf_pages_to_sg()]
>         pci_map_sg()
> 

There is no need to lock these pages.   None of the other direct-IO
code will lock them, and you don't need to either.

The pages are pinned in place via elevated ->count and that is
sufficient.

What you _do_ need to do is to run set_page_dirty() against each
page, before running page_cache_release().  This is only needed
for a read (writing to userspace memory) to tell the VM that the
contents of the page have been altered.

If you don't do this, the VM may steal the page away without writing
it anywhere, and it will be subsequently restored from swap, minus
the changes which are supposed to be there.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [PATCH] Really start using the page walking API

[Douglas Gilbert]

Ingo Oeser wrote:
> Hi all,
> 
> here come some improvements of the page walking API code.
> 
> First: make_pages_present() would do an infinite recursion, if
>    used in find_extend_vma(). I fixed this. Might as well have
>    caused the ntp crash, that has been observed. 
>    So these make_pages_present parts are really important.
> 
> I also did the promised rewrite of make_pages_present() and its
> users.
> 
> MM-Gurus: Please double check, that I always provide the right vma.
> 
> I also did two sample implementations (Kai and Doug, this is why
> you are CC'ed) of the scatter list walking and removed ~100
> lines of code while doing it.

<snip/>
Ingo,
I see that Andrew has put this patch in 2.5.49-mm2 and seems
to be asking for testers. So I will try and test sg's usage.
[It is a while since I built in sg (and other drivers) but
working modules has been very frustrating since 2.5.48 ...]

Doug Gilbert

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/