vm_insert_pfn() is not setting the VM_PFNMAP flag in vma. Fix that.
Signed-off-by: Venkatesh Pallipadi <[email protected]>
Signed-off-by: Suresh Siddha <[email protected]>
---
mm/memory.c | 2 ++
1 file changed, 2 insertions(+)
Index: tip/mm/memory.c
===================================================================
--- tip.orig/mm/memory.c 2008-11-06 09:44:56.000000000 -0800
+++ tip/mm/memory.c 2008-11-10 09:44:47.000000000 -0800
@@ -1444,6 +1444,8 @@ int vm_insert_pfn(struct vm_area_struct
if (addr < vma->vm_start || addr >= vma->vm_end)
return -EFAULT;
+
+ vma->vm_flags |= VM_PFNMAP;
return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
}
EXPORT_SYMBOL(vm_insert_pfn);
--
You have to be careful of this, because it can be called with mmap_sem
held for read only. Hmm, I guess vm_insert_page is doing the same thing.
Probably mostly works because all other modifiers of vm_flags are holding
mmap_sem.
However, in some cases, code can do vm_insert_pfn and vm_insert_page
(actually hmm, no vm_insert_mixed actually should cover most of those
cases).
Still, I'd be much happier if we could make these into BUG_ON, and then
teach callers to set it in their .mmap routines.
On Wed, Nov 12, 2008 at 01:26:49PM -0800, Venkatesh Pallipadi wrote:
> vm_insert_pfn() is not setting the VM_PFNMAP flag in vma. Fix that.
>
> Signed-off-by: Venkatesh Pallipadi <[email protected]>
> Signed-off-by: Suresh Siddha <[email protected]>
>
> ---
> mm/memory.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> Index: tip/mm/memory.c
> ===================================================================
> --- tip.orig/mm/memory.c 2008-11-06 09:44:56.000000000 -0800
> +++ tip/mm/memory.c 2008-11-10 09:44:47.000000000 -0800
> @@ -1444,6 +1444,8 @@ int vm_insert_pfn(struct vm_area_struct
>
> if (addr < vma->vm_start || addr >= vma->vm_end)
> return -EFAULT;
> +
> + vma->vm_flags |= VM_PFNMAP;
> return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
> }
> EXPORT_SYMBOL(vm_insert_pfn);
>
> --
>-----Original Message-----
>From: Nick Piggin [mailto:[email protected]]
>Sent: Wednesday, November 12, 2008 3:23 PM
>To: Pallipadi, Venkatesh
>Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins;
>Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
>Ven; [email protected]; Siddha, Suresh B
>Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
>
>You have to be careful of this, because it can be called with mmap_sem
>held for read only. Hmm, I guess vm_insert_page is doing the
>same thing.
>Probably mostly works because all other modifiers of vm_flags
>are holding
>mmap_sem.
Yes. I did the patch looking at vm_insert_page doing similar thing.
>
>However, in some cases, code can do vm_insert_pfn and vm_insert_page
>(actually hmm, no vm_insert_mixed actually should cover most of those
>cases).
>
>Still, I'd be much happier if we could make these into BUG_ON, and then
>teach callers to set it in their .mmap routines.
Actually, vm_insert_pfn() already has a BUG_ON() at the start for cases
where neither (or both) MIXEDMAP and PFNMAP is not set. So, that should
cover the case we are worried about it here and we can eliminate this
patch altogether. Only part I am not sure about is why we are looking
for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?
Thanks,
Venki
On Wed, Nov 12, 2008 at 04:02:47PM -0800, Pallipadi, Venkatesh wrote:
>
>
> >-----Original Message-----
> >From: Nick Piggin [mailto:[email protected]]
> >Sent: Wednesday, November 12, 2008 3:23 PM
> >To: Pallipadi, Venkatesh
> >Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins;
> >Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
> >Ven; [email protected]; Siddha, Suresh B
> >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
> >
> >You have to be careful of this, because it can be called with mmap_sem
> >held for read only. Hmm, I guess vm_insert_page is doing the
> >same thing.
> >Probably mostly works because all other modifiers of vm_flags
> >are holding
> >mmap_sem.
>
> Yes. I did the patch looking at vm_insert_page doing similar thing.
>
> >
> >However, in some cases, code can do vm_insert_pfn and vm_insert_page
> >(actually hmm, no vm_insert_mixed actually should cover most of those
> >cases).
> >
> >Still, I'd be much happier if we could make these into BUG_ON, and then
> >teach callers to set it in their .mmap routines.
>
> Actually, vm_insert_pfn() already has a BUG_ON() at the start for cases
> where neither (or both) MIXEDMAP and PFNMAP is not set. So, that should
> cover the case we are worried about it here and we can eliminate this
> patch altogether. Only part I am not sure about is why we are looking
> for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?
They should, but it will do an inesrt_pfn in some cases, won't it?
>-----Original Message-----
>From: Nick Piggin [mailto:[email protected]]
>Sent: Wednesday, November 12, 2008 7:44 PM
>To: Pallipadi, Venkatesh
>Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins;
>Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
>Ven; [email protected]; Siddha, Suresh B
>Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
>
>On Wed, Nov 12, 2008 at 04:02:47PM -0800, Pallipadi, Venkatesh wrote:
>>
>>
>> >-----Original Message-----
>> >From: Nick Piggin [mailto:[email protected]]
>> >Sent: Wednesday, November 12, 2008 3:23 PM
>> >To: Pallipadi, Venkatesh
>> >Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins;
>> >Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
>> >Ven; [email protected]; Siddha, Suresh B
>> >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in
>vm_insert_pfn
>> >
>> >You have to be careful of this, because it can be called
>with mmap_sem
>> >held for read only. Hmm, I guess vm_insert_page is doing the
>> >same thing.
>> >Probably mostly works because all other modifiers of vm_flags
>> >are holding
>> >mmap_sem.
>>
>> Yes. I did the patch looking at vm_insert_page doing similar thing.
>>
>> >
>> >However, in some cases, code can do vm_insert_pfn and vm_insert_page
>> >(actually hmm, no vm_insert_mixed actually should cover
>most of those
>> >cases).
>> >
>> >Still, I'd be much happier if we could make these into
>BUG_ON, and then
>> >teach callers to set it in their .mmap routines.
>>
>> Actually, vm_insert_pfn() already has a BUG_ON() at the
>start for cases
>> where neither (or both) MIXEDMAP and PFNMAP is not set. So,
>that should
>> cover the case we are worried about it here and we can eliminate this
>> patch altogether. Only part I am not sure about is why we are looking
>> for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?
>
>They should, but it will do an inesrt_pfn in some cases, won't it?
>
Yes. It does. But, it calls a lower level insert_pfn() function. The lower
level insert_pfn() does not have any bug checks. But the higher level
vm_insert_pfn() checks for PFNMAP or MIXEDMAP.
Thanks,
Venki
On Thu, Nov 13, 2008 at 10:47:23AM -0800, Pallipadi, Venkatesh wrote:
> >> >-----Original Message-----
> >> >From: Nick Piggin [mailto:[email protected]]
> >> >Sent: Wednesday, November 12, 2008 3:23 PM
> >> >To: Pallipadi, Venkatesh
> >> >Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins;
> >> >Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
> >> >Ven; [email protected]; Siddha, Suresh B
> >> >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in
> >vm_insert_pfn
> >> >
> >> >You have to be careful of this, because it can be called
> >with mmap_sem
> >> >held for read only. Hmm, I guess vm_insert_page is doing the
> >> >same thing.
> >> >Probably mostly works because all other modifiers of vm_flags
> >> >are holding
> >> >mmap_sem.
> >>
> >> Yes. I did the patch looking at vm_insert_page doing similar thing.
> >>
> >> >
> >> >However, in some cases, code can do vm_insert_pfn and vm_insert_page
> >> >(actually hmm, no vm_insert_mixed actually should cover
> >most of those
> >> >cases).
> >> >
> >> >Still, I'd be much happier if we could make these into
> >BUG_ON, and then
> >> >teach callers to set it in their .mmap routines.
> >>
> >> Actually, vm_insert_pfn() already has a BUG_ON() at the
> >start for cases
> >> where neither (or both) MIXEDMAP and PFNMAP is not set. So,
> >that should
> >> cover the case we are worried about it here and we can eliminate this
> >> patch altogether. Only part I am not sure about is why we are looking
> >> for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?
> >
> >They should, but it will do an inesrt_pfn in some cases, won't it?
> >
>
> Yes. It does. But, it calls a lower level insert_pfn() function. The lower
> level insert_pfn() does not have any bug checks. But the higher level
> vm_insert_pfn() checks for PFNMAP or MIXEDMAP.
Yes, but is there anything extra you need to check for cache aliases in
MIXEDMAP mappings?
>-----Original Message-----
>From: Nick Piggin [mailto:[email protected]]
>Sent: Thursday, November 13, 2008 6:06 PM
>To: Pallipadi, Venkatesh
>Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins;
>Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
>Ven; [email protected]; Siddha, Suresh B
>Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
>
>On Thu, Nov 13, 2008 at 10:47:23AM -0800, Pallipadi, Venkatesh wrote:
>>
>> Yes. It does. But, it calls a lower level insert_pfn()
>function. The lower
>> level insert_pfn() does not have any bug checks. But the higher level
>> vm_insert_pfn() checks for PFNMAP or MIXEDMAP.
>
>Yes, but is there anything extra you need to check for cache aliases in
>MIXEDMAP mappings?
>
Yes. We need additional things to track MIXEDMAP and we are looking at that.
But, that is slightly more trickier than the general PFNMAP case. And
only in-tree user of MIXEDMAP is xip and that too it only uses it for
regular WB mapping. So, we thought we should fix the more common case
first here.
With MIXEDMAP there is no way whether to distinguish whether insert_pfn
Or insert_page was used while looking at VMA. We can probably use PFNMAP
in addition to MIXEDMAP to indicate that, which will make things easier.
But, we are still looking at that and trying to understand the change
implication.
Thanks,
Venki
On Thu, 2008-11-13 at 00:23 +0100, Nick Piggin wrote:
> You have to be careful of this, because it can be called with mmap_sem
> held for read only. Hmm, I guess vm_insert_page is doing the same thing.
> Probably mostly works because all other modifiers of vm_flags are holding
> mmap_sem.
>
> However, in some cases, code can do vm_insert_pfn and vm_insert_page
> (actually hmm, no vm_insert_mixed actually should cover most of those
> cases).
>
> Still, I'd be much happier if we could make these into BUG_ON, and then
> teach callers to set it in their .mmap routines.
Agreed. I don't think vm_insert_* is the right place to muck
with that ..
Ben.
>
> On Wed, Nov 12, 2008 at 01:26:49PM -0800, Venkatesh Pallipadi wrote:
> > vm_insert_pfn() is not setting the VM_PFNMAP flag in vma. Fix that.
> >
> > Signed-off-by: Venkatesh Pallipadi <[email protected]>
> > Signed-off-by: Suresh Siddha <[email protected]>
> >
> > ---
> > mm/memory.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > Index: tip/mm/memory.c
> > ===================================================================
> > --- tip.orig/mm/memory.c 2008-11-06 09:44:56.000000000 -0800
> > +++ tip/mm/memory.c 2008-11-10 09:44:47.000000000 -0800
> > @@ -1444,6 +1444,8 @@ int vm_insert_pfn(struct vm_area_struct
> >
> > if (addr < vma->vm_start || addr >= vma->vm_end)
> > return -EFAULT;
> > +
> > + vma->vm_flags |= VM_PFNMAP;
> > return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
> > }
> > EXPORT_SYMBOL(vm_insert_pfn);
> >
> > --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Fri, Nov 14, 2008 at 01:35:38PM -0800, Pallipadi, Venkatesh wrote:
>
>
> >-----Original Message-----
> >From: Nick Piggin [mailto:[email protected]]
> >Sent: Thursday, November 13, 2008 6:06 PM
> >To: Pallipadi, Venkatesh
> >Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins;
> >Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
> >Ven; [email protected]; Siddha, Suresh B
> >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
> >
> >On Thu, Nov 13, 2008 at 10:47:23AM -0800, Pallipadi, Venkatesh wrote:
> >>
> >> Yes. It does. But, it calls a lower level insert_pfn()
> >function. The lower
> >> level insert_pfn() does not have any bug checks. But the higher level
> >> vm_insert_pfn() checks for PFNMAP or MIXEDMAP.
> >
> >Yes, but is there anything extra you need to check for cache aliases in
> >MIXEDMAP mappings?
> >
>
> Yes. We need additional things to track MIXEDMAP and we are looking at that.
> But, that is slightly more trickier than the general PFNMAP case. And
> only in-tree user of MIXEDMAP is xip and that too it only uses it for
> regular WB mapping. So, we thought we should fix the more common case
> first here.
>
> With MIXEDMAP there is no way whether to distinguish whether insert_pfn
> Or insert_page was used while looking at VMA. We can probably use PFNMAP
> in addition to MIXEDMAP to indicate that, which will make things easier.
It's difficult because it can have either method for a single VMA, and
a given address in the vma may even change over time (not with current
code in kernel AFAIKS, but AXFS eventually might get to that point).
> But, we are still looking at that and trying to understand the change
> implication.
OK: now I understand correctly. Getting PFNMAP working is an important
first step. I agree.
Thanks,
Nick
* Nick Piggin <[email protected]> wrote:
> On Fri, Nov 14, 2008 at 01:35:38PM -0800, Pallipadi, Venkatesh wrote:
> >
> >
> > >-----Original Message-----
> > >From: Nick Piggin [mailto:[email protected]]
> > >Sent: Thursday, November 13, 2008 6:06 PM
> > >To: Pallipadi, Venkatesh
> > >Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins;
> > >Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
> > >Ven; [email protected]; Siddha, Suresh B
> > >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
> > >
> > >On Thu, Nov 13, 2008 at 10:47:23AM -0800, Pallipadi, Venkatesh wrote:
> > >>
> > >> Yes. It does. But, it calls a lower level insert_pfn()
> > >function. The lower
> > >> level insert_pfn() does not have any bug checks. But the higher level
> > >> vm_insert_pfn() checks for PFNMAP or MIXEDMAP.
> > >
> > >Yes, but is there anything extra you need to check for cache aliases in
> > >MIXEDMAP mappings?
> > >
> >
> > Yes. We need additional things to track MIXEDMAP and we are looking at that.
> > But, that is slightly more trickier than the general PFNMAP case. And
> > only in-tree user of MIXEDMAP is xip and that too it only uses it for
> > regular WB mapping. So, we thought we should fix the more common case
> > first here.
> >
> > With MIXEDMAP there is no way whether to distinguish whether insert_pfn
> > Or insert_page was used while looking at VMA. We can probably use PFNMAP
> > in addition to MIXEDMAP to indicate that, which will make things easier.
>
> It's difficult because it can have either method for a single VMA, and
> a given address in the vma may even change over time (not with current
> code in kernel AFAIKS, but AXFS eventually might get to that point).
>
>
> > But, we are still looking at that and trying to understand the change
> > implication.
>
> OK: now I understand correctly. Getting PFNMAP working is an important
> first step. I agree.
Venki, a patch logistics sidenote: the final mm/* bits of this
patchset need acks from MM folks - Andrew, Nick or Hugh - we cannot
just queue them up in the x86/pat tree without agreement from MM
maintainers.
Ingo
>-----Original Message-----
>From: [email protected]
>[mailto:[email protected]] On Behalf Of Ingo Molnar
>Sent: Tuesday, November 18, 2008 1:38 PM
>To: Nick Piggin
>Cc: Pallipadi, Venkatesh; Thomas Gleixner; H Peter Anvin; Hugh
>Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge;
>Arjan van de Ven; [email protected]; Siddha, Suresh B
>Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
>
>
>* Nick Piggin <[email protected]> wrote:
>
>> On Fri, Nov 14, 2008 at 01:35:38PM -0800, Pallipadi, Venkatesh wrote:
>> >
>> >
>> > >-----Original Message-----
>> > >From: Nick Piggin [mailto:[email protected]]
>> > >Sent: Thursday, November 13, 2008 6:06 PM
>> > >To: Pallipadi, Venkatesh
>> > >Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins;
>> > >Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de
>> > >Ven; [email protected]; Siddha, Suresh B
>> > >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in
>vm_insert_pfn
>> > >
>> > >On Thu, Nov 13, 2008 at 10:47:23AM -0800, Pallipadi,
>Venkatesh wrote:
>> > >>
>> > >> Yes. It does. But, it calls a lower level insert_pfn()
>> > >function. The lower
>> > >> level insert_pfn() does not have any bug checks. But
>the higher level
>> > >> vm_insert_pfn() checks for PFNMAP or MIXEDMAP.
>> > >
>> > >Yes, but is there anything extra you need to check for
>cache aliases in
>> > >MIXEDMAP mappings?
>> > >
>> >
>> > Yes. We need additional things to track MIXEDMAP and we
>are looking at that.
>> > But, that is slightly more trickier than the general
>PFNMAP case. And
>> > only in-tree user of MIXEDMAP is xip and that too it only
>uses it for
>> > regular WB mapping. So, we thought we should fix the more
>common case
>> > first here.
>> >
>> > With MIXEDMAP there is no way whether to distinguish
>whether insert_pfn
>> > Or insert_page was used while looking at VMA. We can
>probably use PFNMAP
>> > in addition to MIXEDMAP to indicate that, which will make
>things easier.
>>
>> It's difficult because it can have either method for a
>single VMA, and
>> a given address in the vma may even change over time (not
>with current
>> code in kernel AFAIKS, but AXFS eventually might get to that point).
>>
>>
>> > But, we are still looking at that and trying to understand
>the change
>> > implication.
>>
>> OK: now I understand correctly. Getting PFNMAP working is an
>important
>> first step. I agree.
>
>Venki, a patch logistics sidenote: the final mm/* bits of this
>patchset need acks from MM folks - Andrew, Nick or Hugh - we cannot
>just queue them up in the x86/pat tree without agreement from MM
>maintainers.
>
OK. Feedback based on the discussions on this patchset, this particular patch
set VM_PFNMAP flag in vm_insert_pfn
is not really needed and we should rather BUG_ON on that not being set.
And there were few more comments on one other generic vm patch from Nick.
I will resend the entire patchset with changes soon and then we can work
out the logistics.
Thanks,
Venki-
On Thu, Nov 20, 2008 at 03:42:32PM -0800, Pallipadi, Venkatesh wrote:
>
>
> >-----Original Message-----
> >From: [email protected]
> >[mailto:[email protected]] On Behalf Of Ingo Molnar
> >Sent: Tuesday, November 18, 2008 1:38 PM
> >To: Nick Piggin
> >Cc: Pallipadi, Venkatesh; Thomas Gleixner; H Peter Anvin; Hugh
> >Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge;
> >Arjan van de Ven; [email protected]; Siddha, Suresh B
> >Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn
> >
> >
> >patchset need acks from MM folks - Andrew, Nick or Hugh - we cannot
> >just queue them up in the x86/pat tree without agreement from MM
> >maintainers.
> >
>
> OK. Feedback based on the discussions on this patchset, this particular patch
> set VM_PFNMAP flag in vm_insert_pfn
> is not really needed and we should rather BUG_ON on that not being set.
> And there were few more comments on one other generic vm patch from Nick.
>
> I will resend the entire patchset with changes soon and then we can work
> out the logistics.
So long as it has been reviewed by mm people (cc'ing linux-mm might be
nice), I don't care so much how it makes its way upstream. If it was
a lot of code, it might make sense to slowly work dependencies though
various subsystems... but this isn't too much on the mm side, and it is
primarily support code for x86 subsystem feature so I don't mind myself
if it went via the x86 tree, with appropriate acks.