Interaction between Xen and XFS: stray RW mappings
Subsystems (3)
Hi Dave & other XFS folk,
I'm tracking down a bug which appears to be a bad interaction between XFS and Xen. It looks like XFS is holding RW mappings on free pages, which Xen is trying to get an exclusive RO mapping on so it can turn them into pagetables.
I'm assuming the pages are actually free, and this isn't a use after free problem. From a quick poke around, the most likely pieces of XFS code is the stuff in xfs_map.c which creates a virtually contiguous mapping of pages with vmap, and seems to delay unmapping them.
When pinning a pagetable, Xen tries to eliminate any RW aliases of the pages its using. This is generally trivial because pages returned by get_free_page don't have any mappings aside from the normal kernel mapping. High pages, when using CONFIG_HIGHPTE, may have a residual kmap mapping, so we clear out the kmap cache if we encounter a highpage in the pagetable.
I guess we could create a special-case interface to do the same thing with XFS mappings, but it would be nicer to have something more generic.
Is my analysis correct? Or should XFS not be holding stray mappings? Or is there already some kind of generic mechanism I can use to get it to release its mappings?
Thanks,
J
Quoted 6 lines · Jeremy Fitzhardinge
I guess we could create a special-case interface to do the same thing with XFS mappings, but it would be nicer to have something more generic.
Is my analysis correct? Or should XFS not be holding stray mappings? Or is there already some kind of generic mechanism I can use to get it to release its mappings?
This test patch confirms my theory:
fs/xfs/linux-2.6/xfs_buf.c +5 -0
@@ -186,6 +186,11 @@ free_address( void *addr) {a_list_t *aentry; + +#ifdef CONFIG_XEN +vunmap (addr); +return ; +#endif aentry =kmalloc (sizeof (a_list_t),GFP_NOWAIT );if (likely (aentry)) {
With this in place, the problem goes away.
J
Quoted 11 lines · Jeremy Fitzhardinge
Hi Dave & other XFS folk,
I'm tracking down a bug which appears to be a bad interaction between XFS and Xen. It looks like XFS is holding RW mappings on free pages, which Xen is trying to get an exclusive RO mapping on so it can turn them into pagetables.
I'm assuming the pages are actually free, and this isn't a use after free problem. From a quick poke around, the most likely pieces of XFS code is the stuff in
xfs_map.cwhich creates a virtually contiguous mapping of pages with vmap, and seems to delay unmapping them.
You mean xfs_buf.c.
And yes, we delay unmapping pages until we have a batch of them to unmap. vmap and vunmap do not scale, so this is batching helps alleviate some of the worst of the problems.
Quoted 8 lines
When pinning a pagetable, Xen tries to eliminate any RW aliases of the pages its using. This is generally trivial because pages returned by
get_free_pagedon't have any mappings aside from the normal kernel mapping. High pages, when usingCONFIG_HIGHPTE, may have a residual kmap mapping, so we clear out the kmap cache if we encounter a highpage in the pagetable.I guess we could create a special-case interface to do the same thing with XFS mappings, but it would be nicer to have something more generic.
*nod*
Quoted 3 lines
Is my analysis correct? Or should XFS not be holding stray mappings? Or is there already some kind of generic mechanism I can use to get it to release its mappings?
The xfsbufd cleans out any stale mappings - it's woken by the memory shrinker interface (i.e. calls xfsbufd_wakeup()). Otherwise every 64th buffer being vmap()d will flush out stale mappings.
Realistically, if this delayed release of vmaps is a problem for Xen, then I think that some generic VM solution is needed to this problem as vmap() is likely to become more common in future (think large blocks in filesystems). Nick - any comments?
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 2 lines · David Chinner
You mean
xfs_buf.c.
Yes, sorry.
Quoted 4 lines
And yes, we delay unmapping pages until we have a batch of them to unmap. vmap and vunmap do not scale, so this is batching helps alleviate some of the worst of the problems.
How much performance does it cost? What kind of workloads would it show up under?
Quoted 5 lines
Realistically, if this delayed release of vmaps is a problem for Xen, then I think that some generic VM solution is needed to this problem as
vmap()is likely to become more common in future (think large blocks in filesystems). Nick - any comments?
Well, the only real problem is that the pages are returned to the free pool and reallocated while still being part of a mapping. If the pages are still owned by the filesystem/pagecache, then there's no problem.
What's the lifetime of things being vmapped/unmapped in xfs? Are they necessarily being freed when they're unmapped, or could unmapping of freed memory be more immediate than other memory?
Maybe it just needs a notifier chain or something.
J
Quoted 17 lines · Jeremy Fitzhardinge
Quoted 1 line · David Chinner
You mean
xfs_buf.c.Yes, sorry.
Quoted 3 lines
And yes, we delay unmapping pages until we have a batch of them to unmap. vmap and vunmap do not scale, so this is batching helps alleviate some of the worst of the problems.
How much performance does it cost? What kind of workloads would it show up under?
Quoted 4 lines
Realistically, if this delayed release of vmaps is a problem for Xen, then I think that some generic VM solution is needed to this problem as
vmap()is likely to become more common in future (think large blocks in filesystems). Nick - any comments?
Quoted 7 lines
Well, the only real problem is that the pages are returned to the free pool and reallocated while still being part of a mapping. If the pages are still owned by the filesystem/pagecache, then there's no problem.
What's the lifetime of things being vmapped/unmapped in xfs? Are they necessarily being freed when they're unmapped, or could unmapping of freed memory be more immediate than other memory?
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
I'm looking at some more general solutions to this (already have some batching / lazy unmapping that replaces the XFS specific one), however they are still likely going to leave vmap mappings around after freeing the page.
We _could_ hold on to the pages as well, but that's pretty inefficient. The memory cost of keeping the mappings around tends to be well under 1% the cost of the page itself. OTOH we could also avoid lazy flushes on architectures where it is not costly. Either way, it probably would require an arch hook or even a couple of ifdefs in mm/vmalloc.c for Xen. Although... it would be nice if Xen could take advantage of some of these optimisations as well.
What's the actual problem for Xen? Anything that can be changed?
Quoted 4 lines · Jeremy Fitzhardinge
Quoted 2 lines · David Chinner
You mean
xfs_buf.c.
Quoted 8 lines
Yes, sorry.
Quoted 4 lines
And yes, we delay unmapping pages until we have a batch of them to unmap. vmap and vunmap do not scale, so this is batching helps alleviate some of the worst of the problems.
How much performance does it cost?
Every vunmap() cal causes a global TLB sync, and the region lists are globl with a spin lock protecting them. I thin kNick has shown a 64p altix with ~60 cpus spinning on the vmap locks under a simple workload....
Quoted 2 lines
What kind of workloads would it show
up under?
A directory traversal when using large directory block sizes with large directories....
Quoted 9 lines
Quoted 5 lines
Realistically, if this delayed release of vmaps is a problem for Xen, then I think that some generic VM solution is needed to this problem as
vmap()is likely to become more common in future (think large blocks in filesystems). Nick - any comments?Well, the only real problem is that the pages are returned to the free pool and reallocated while still being part of a mapping. If the pages are still owned by the filesystem/pagecache, then there's no problem.
The pages are still attached to the blockdev address space mapping, but there's nothing stopping them from being reclaimed before they are unmapped.
Quoted 3 lines
What's the lifetime of things being vmapped/unmapped in xfs? Are they necessarily being freed when they're unmapped, or could unmapping of freed memory be more immediate than other memory?
It's all "freed memory". At the time we pull the buffer down, there are no further references to the buffer. the pages are released and the mapping is never used again until it is torn down. it is torn down either on the next xfsbufd run (either memory pressure or every 15s) or every 64th new vmap() call to map new buffers.
Quoted 1 line
Maybe it just needs a notifier chain or something.
We've already got a memroy shrinker hook that triggers this reclaim.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 3 lines · Nick Piggin
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
I see.
Quoted 5 lines
I'm looking at some more general solutions to this (already have some batching / lazy unmapping that replaces the XFS specific one), however they are still likely going to leave vmap mappings around after freeing the page.
Hm. Could there be a call to shoot down any lazy mappings of a page, so the Xen pagetable code could use it on any pagetable page? Ideally one that could be used on any page, but only causes expensive operations where needed.
Quoted 8 lines
We _could_ hold on to the pages as well, but that's pretty inefficient. The memory cost of keeping the mappings around tends to be well under 1% the cost of the page itself. OTOH we could also avoid lazy flushes on architectures where it is not costly. Either way, it probably would require an arch hook or even a couple of ifdefs in
mm/vmalloc.cfor Xen. Although... it would be nice if Xen could take advantage of some of these optimisations as well.
In general the lazy unmappings won't worry Xen. It's only for the specific case of allocating memory for pagetables. Xen can do a bit of extra optimisation for cross-cpu tlb flushes (if the target vcpus are not currently running, then you don't need to do anything), but they're still an expensive operation, so the optimisation is definitely useful.
Quoted 2 lines
What's the actual problem for Xen? Anything that can be changed?
Not easily. Xen doesn't use shadow pagetables. Instead, it gives the guest domains direct access to the real CPU's pagetable, but makes sure they're always mapped RO so that the hypervisor can control updates to the pagetables (either by trapping writes or via explicit hypercalls). This means that when constructing a new pagetable, Xen will verify that all the mappings of pages making up the new pagetable are RO before allowing it to be used. If there are stray RW mappings of those pages, pagetable construction will fail.
Aside from XFS, the only other case I've found where there could be stray RW mappings is when using high pages which are still in the kmap cache; I added an explicit call to flush the kmap cache to handle this. If vmap and kmap can be unified (at least the lazy unmap aspects of them), then that would be a nice little cleanup.
J
Quoted 11 lines · Jeremy Fitzhardinge
Quoted 2 lines · Nick Piggin
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
I see.
Quoted 4 lines
I'm looking at some more general solutions to this (already have some batching / lazy unmapping that replaces the XFS specific one), however they are still likely going to leave vmap mappings around after freeing the page.
Quoted 4 lines
Hm. Could there be a call to shoot down any lazy mappings of a page, so the Xen pagetable code could use it on any pagetable page? Ideally one that could be used on any page, but only causes expensive operations where needed.
Yeah, it would be possible. The easiest way would just be to shoot down all lazy vmaps (because you're doing the global IPIs anyway, which are the expensive thing, at which point you may as well purge the rest of your lazy mappings).
If it is sufficiently rare, then it could be the simplest thing to do.
Quoted 8 lines
Quoted 7 lines
We _could_ hold on to the pages as well, but that's pretty inefficient. The memory cost of keeping the mappings around tends to be well under 1% the cost of the page itself. OTOH we could also avoid lazy flushes on architectures where it is not costly. Either way, it probably would require an arch hook or even a couple of ifdefs in
mm/vmalloc.cfor Xen. Although... it would be nice if Xen could take advantage of some of these optimisations as well.
Quoted 5 lines
In general the lazy unmappings won't worry Xen. It's only for the specific case of allocating memory for pagetables. Xen can do a bit of extra optimisation for cross-cpu tlb flushes (if the target vcpus are not currently running, then you don't need to do anything), but they're still an expensive operation, so the optimisation is definitely useful.
OK.
Quoted 10 lines
Quoted 1 line
What's the actual problem for Xen? Anything that can be changed?
Not easily. Xen doesn't use shadow pagetables. Instead, it gives the guest domains direct access to the real CPU's pagetable, but makes sure they're always mapped RO so that the hypervisor can control updates to the pagetables (either by trapping writes or via explicit hypercalls). This means that when constructing a new pagetable, Xen will verify that all the mappings of pages making up the new pagetable are RO before allowing it to be used. If there are stray RW mappings of those pages, pagetable construction will fail.
OK, I see. Because even though it is technically safe where we are using it (because nothing writes through the mappings after the page is freed), a corrupted guest could use the same window to do bad things with the pagetables?
Quoted 5 lines
Aside from XFS, the only other case I've found where there could be stray RW mappings is when using high pages which are still in the kmap cache; I added an explicit call to flush the kmap cache to handle this. If vmap and kmap can be unified (at least the lazy unmap aspects of them), then that would be a nice little cleanup.
vmap is slightly harder than kmap in some respects. However it would be really nice to get vmap fast and general enough to completely replace all the kmap crud -- that's one goal, but the first thing I'm doing is to concentrate on just vmap to work out how to make it as fast as possible.
For Xen -- shouldn't be a big deal. We can have a single Linux mm API to call, and we can do the right thing WRT vmap/kamp. I should try to merge my current lazy vmap patches which replace the XFS stuff, so we can implement such an API and fix your XFS issue? That's not going to happen for at least a cycle or two though, so in the meantime maybe an ifdef for that XFS vmap batching code would help?
Quoted 5 lines · Nick Piggin
Yeah, it would be possible. The easiest way would just be to shoot down all lazy vmaps (because you're doing the global IPIs anyway, which are the expensive thing, at which point you may as well purge the rest of your lazy mappings).
Sure.
Quoted 2 lines
If it is sufficiently rare, then it could be the simplest thing to do.
Yes. If there's some way to tell whether a particular page is in a lazy mapping then that would help, since we could easily tell whether we need to do the whole shootdown thing. I would expect the population of lazily mapped pages in the whole freepage pool to be pretty small, but if the allocator tends to return the most recently freed pages you might hit them fairly regularly (shoving them at the other end of the freelist might be useful).
Quoted 5 lines
OK, I see. Because even though it is technically safe where we are using it (because nothing writes through the mappings after the page is freed), a corrupted guest could use the same window to do bad things with the pagetables?
That's right. The hypervisor doesn't trust the guests, so it prevents them from getting into a state where they can do bad things.
Quoted 4 lines
For Xen -- shouldn't be a big deal. We can have a single Linux mm API to call, and we can do the right thing WRT vmap/kamp. I should try to merge my current lazy vmap patches which replace the XFS stuff, so we can implement such an API and fix your XFS issue?
Sounds good.
Quoted 4 lines
That's not going to
happen for at least a cycle or two though, so in the meantime maybe an ifdef for that XFS vmap batching code would help?
For now I've proposed a patch to simply eagerly vunmap everything when CONFIG_XEN is set. It certainly works, but I don't have a good feel for how much of a performance hit that imposes on XFS. A slightly more subtle change would be to test to see if we're actually running under Xen before taking the eager path, so at least the performance burden only affects actual Xen users (and I presume xfs+xen is a fairly rare combination, or this problem would have turned up earlier, or perhaps the old xenified kernels have some other workaround for it).
J
Quoted 9 lines · Jeremy Fitzhardinge
Quoted 4 lines · Nick Piggin
That's not going to
happen for at least a cycle or two though, so in the meantime maybe an ifdef for that XFS vmap batching code would help?For now I've proposed a patch to simply eagerly vunmap everything when
CONFIG_XENis set. It certainly works, but I don't have a good feel for how much of a performance hit that imposes on XFS.
With defaults - little effect as vmap should never be used. It's only when you start using larger block sizes for metadata that this becomes an issue. The CONFIG_XEN workaround should be fine until we get a proper vmap cache....
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 4 lines · David Chinner
With defaults - little effect as vmap should never be used. It's only when you start using larger block sizes for metadata that this becomes an issue. The
CONFIG_XENworkaround should be fine until we get a proper vmap cache....
Hm, well I saw the problem with a filesystem made with mkfs.xfs with no options, so there must be at least *some* vmapping going on there.
J
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 4 lines · David Chinner
With defaults - little effect as vmap should never be used. It's only when you start using larger block sizes for metadata that this becomes an issue. The
CONFIG_XENworkaround should be fine until we get a proper vmap cache....Hm, well I saw the problem with a filesystem made with mkfs.xfs with no options, so there must be at least *some* vmapping going on there.
Sorry - I should have been more precise - vmap should never be used in performance critical paths on default configs. Log recovery will trigger vmap/vunmap usage, so this is probably what you are seeing.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 6 lines · David Chinner
Quoted 2 lines
Hm, well I saw the problem with a filesystem made with mkfs.xfs with no options, so there must be at least *some* vmapping going on there.
Sorry - I should have been more precise - vmap should never be used in performance critical paths on default configs. Log recovery will trigger vmap/vunmap usage, so this is probably what you are seeing.
The iclogs are also vmapped, but they aren't unmapped until unmount so this optimizations doesn't matter either.
Quoted 4 lines · David Chinner
And yes, we delay unmapping pages until we have a batch of them to unmap. vmap and vunmap do not scale, so this is batching helps alleviate some of the worst of the problems.
You're keeping vmaps around for already freed pages?
That will be a big problem for proper PAT support, which needs to track all mappings to memory. It's not just a problem for Xen.
In fact I suspect it is already broken with DRM or AGP for example which can use UC and WC mappings -- if you keep the mapping around and DRM or AGP turns the page in another mapping uncacheable you're creating an illegal cache attribute alias. These are known to occasionally create cache corruptions on several x86s; giving ___VERY___ hard to debug bugs once a blue moon.
Probably it'll require some generic VM batching mechanism where Xen or PAT code can hook into the list or force unmap the mappings as needed.
Definitely needs to be fixed if true. You're lucky that Xen caught it in time.
-Andi
Quoted 10 lines · Andi Kleen
Quoted 3 lines · David Chinner
And yes, we delay unmapping pages until we have a batch of them to unmap. vmap and vunmap do not scale, so this is batching helps alleviate some of the worst of the problems.
You're keeping vmaps around for already freed pages?
That will be a big problem for proper PAT support, which needs to track all mappings to memory. It's not just a problem for Xen.
Quoted 6 lines
In fact I suspect it is already broken with DRM or AGP for example which can use UC and WC mappings -- if you keep the mapping around and DRM or AGP turns the page in another mapping uncacheable you're creating an illegal cache attribute alias. These are known to occasionally create cache corruptions on several x86s; giving
___VERY___hard to debug bugs once a blue moon.
Is this true even if you don't write through those old mappings? Is DRM or AGP then not also broken with lazy highmem flushing, or how do they solve that?
Quoted 3 lines
Probably it'll require some generic VM batching mechanism where Xen or PAT code can hook into the list or force unmap the mappings as needed.
Definitely.
Quoted 2 lines
Definitely needs to be fixed if true. You're lucky that Xen caught it in time.
I've been thinking that a simple debug mode could be a good idea. Have a new field in the struct page to count the number of possible unflushed mappings to the page so that things like PAT could go BUG_ON appropriate conditions. Would that be helpful?
Quoted 1 line · Nick Piggin
Is this true even if you don't write through those old mappings?
I think it happened for reads too. It is a little counter intuitive because in theory the CPU doesn't need to write back non dirty lines, but in the one case which took so long to debug exactly this happened somehow.
At it is undefined for reads and writes in the architecture so better be safe than sorry.
And x86 CPUs are out of order and do speculative executation and that can lead to arbitary memory accesses even if the code never touches an particular address.
Newer Intel CPUs have something called self-snoop which was supposed to handle this; but in some situations it doesn't seem to catch it either.
Quoted 2 lines
Is DRM or AGP then not also broken with lazy highmem flushing, or how do they solve that?
AGP doesn't allocate highmem pages. Not sure about the DRM code.
-Andi
Quoted 10 lines · Andi Kleen
Quoted 1 line · Nick Piggin
Is this true even if you don't write through those old mappings?
I think it happened for reads too. It is a little counter intuitive because in theory the CPU doesn't need to write back non dirty lines, but in the one case which took so long to debug exactly this happened somehow.
At it is undefined for reads and writes in the architecture so better be safe than sorry.
Yes, typo. I meant reads or writes.
Quoted 7 lines
And x86 CPUs are out of order and do speculative executation and that can lead to arbitary memory accesses even if the code never touches an particular address.
Newer Intel CPUs have something called self-snoop which was supposed to handle this; but in some situations it doesn't seem to catch it either.
Fair enough, so we have to have this lazy tlb flush hook for Xen/PAT/etc. I don't think it should be much problem to implement.
Quoted 4 lines
Quoted 2 lines
Is DRM or AGP then not also broken with lazy highmem flushing, or how do they solve that?
AGP doesn't allocate highmem pages. Not sure about the DRM code.
Hmm, OK. It looks like DRM vmallocs memory (which gives highmem).
Quoted 1 line
Hmm, OK. It looks like DRM vmallocs memory (which gives highmem).
I meant I'm not sure if it uses that memory uncached. I admit not quite understanding that code. There used to be at least one place where it set UC for an user mapping though.
-Andi
Quoted 5 lines · Andi Kleen
Quoted 1 line
Hmm, OK. It looks like DRM vmallocs memory (which gives highmem).
I meant I'm not sure if it uses that memory uncached. I admit not quite understanding that code. There used to be at least one place where it set UC for an user mapping though.
Currently the only DRM memory handed to userspace is vmalloc_32 in drm_scatter.c
I notice the VIA driver does its own vmalloc for dmablit, so it may have an issue with this if highmem is involved.
This will change with the upcoming memory manager so I'll need to investigate it a bit perhaps...
Dave.
Quoted 7 lines · Andi Kleen
Quoted 1 line · Nick Piggin
Is this true even if you don't write through those old mappings?
I think it happened for reads too. It is a little counter intuitive because in theory the CPU doesn't need to write back non dirty lines, but in the one case which took so long to debug exactly this happened somehow.
The problem exist also on ppc, and afaik, is due to the line being in the cache at all (either dirty (write) or not (read)), thus causing the snoop logic to hit, that is what's causing the problem vs. non cached accesses.
Also, on some processors, the simple fact of having the page mapped can cause the CPU to prefetch from it even if it's not actually accessed (speculative prefetch can cross page boundaries if things are mapped).
Ben.
Quoted 2 lines · Nick Piggin
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
why is that? ignoring 32-bit archs we have heaps of address space available... couldn't the kernel just burn address space and delay global TLB invalidate by some relatively long time (say 1 second)?
-dean
Quoted 11 lines · dean gaudet
Quoted 3 lines · Nick Piggin
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
why is that? ignoring 32-bit archs we have heaps of address space available... couldn't the kernel just burn address space and delay global TLB invalidate by some relatively long time (say 1 second)?
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
J
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 7 lines · dean gaudet
Quoted 3 lines · Nick Piggin
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
Quoted 7 lines
Quoted 4 lines
why is that? ignoring 32-bit archs we have heaps of address space available... couldn't the kernel just burn address space and delay global TLB invalidate by some relatively long time (say 1 second)?
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
sounds like a bug in xen to me :)
-dean
Quoted 6 lines · dean gaudet
Quoted 5 lines · Jeremy Fitzhardinge
Quoted 4 lines · dean gaudet
Quoted 2 lines · Nick Piggin
Yes, as Dave said, vmap (more specifically: vunmap) is very expensive because it generally has to invalidate TLBs on all CPUs.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
why is that? ignoring 32-bit archs we have heaps of address space available... couldn't the kernel just burn address space and delay global TLB invalidate by some relatively long time (say 1 second)?
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
sounds like a bug in xen to me :)
You could call it a bug I think. I don't know much about Xen though, whether or not it expects to be able to run an arbitrary OS kernel.
Presumably, the hypervisor _could_ write protect and trap writes to *all* page table page mappings.
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 7 lines · Andi Kleen
Quoted 1 line · Nick Piggin
Is this true even if you don't write through those old mappings?
I think it happened for reads too. It is a little counter intuitive because in theory the CPU doesn't need to write back non dirty lines, but in the one case which took so long to debug exactly this happened somehow.
Quoted 4 lines
The problem exist also on ppc, and afaik, is due to the line being in the cache at all (either dirty (write) or not (read)), thus causing the snoop logic to hit, that is what's causing the problem vs. non cached accesses.
That makes sense. Snoop can effectively turn a read into a write.
Quoted 3 lines
Also, on some processors, the simple fact of having the page mapped can cause the CPU to prefetch from it even if it's not actually accessed (speculative prefetch can cross page boundaries if things are mapped).
Exactly that happens on x86. Normally prefetches stop on TLB miss, but the CPU can do speculative TLB fetches too.
Also even without any prefetching the CPU does speculative execution and that can lead to random addresses being followed.
-Andi
Quoted 3 lines · Jeremy Fitzhardinge
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
Again it not just upsets Xen, keeping mappings to freed pages is wrong generally and violates the x86 (and likely others like PPC) architecture because it can cause illegal caching attribute aliases.
The patch that went into the tree was really not correct -- this bogus optimization should have been unconditionally removed or if you really wanted an ifdef made dependent on !CONFIG_XEN && !CONFIG_AGP (and likely && !CONFIG_DRM && !CONFIG_anything else that uses uncached mappings in memory).
You just worked around the obvious failure and leave the non obvious rare corruptions in, which isn't a good strategy.
-Andi
Quoted 2 lines · dean gaudet
sounds like a bug in xen to me :)
I explained at the head of this thread how and why Xen works in this manner. It's certainly a change from native execution; whether you consider it to be a bug is a different matter.
But it turns out that leaving stray mappings around on pages which get later used in special ways is a bad idea, and can manifest in all kinds of random unpleasant ways. Getting a clear error out of Xen is probably nicest way to discover and debug this problem.
J
Quoted 3 lines · Nick Piggin
You could call it a bug I think. I don't know much about Xen though, whether or not it expects to be able to run an arbitrary OS kernel.
Xen's paravirtualized mode always requires a guest OS to be modified; certainly some operating systems would be very hard to make work with Xen. But you can always fall back to using shadow pagetables or full hvm (VT/SVM) mode.
Quoted 3 lines
Presumably, the hypervisor _could_ write protect and trap writes to *all* page table page mappings.
Xen manages this stuff with refcounts; it doesn't maintain an rmap for these pages, so it would have to exhaustively search to do this. But aside from that, Xen never actively modifies pagetables, so this would be a new behaviour in the interface.
J
Quoted 10 lines · Andi Kleen
Quoted 3 lines · Jeremy Fitzhardinge
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
Again it not just upsets Xen, keeping mappings to freed pages is wrong generally and violates the x86 (and likely others like PPC) architecture because it can cause illegal caching attribute aliases.
Quoted 8 lines
The patch that went into the tree was really not correct -- this bogus optimization should have been unconditionally removed or if you really wanted an ifdef made dependent on !CONFIG_XEN && !CONFIG_AGP (and likely && !CONFIG_DRM && !CONFIG_anything else that uses uncached mappings in memory).
You just worked around the obvious failure and leave the non obvious rare corruptions in, which isn't a good strategy.
Well, at least it becomes a known issue and/or placeholder for when Nick does his grand unified vmap manager. I guess a clean workaround would be to add a CONFIG_XFS_LAZY_UNMAP, and do it at the Kconfig level... I'll cook up a patch.
J
Quoted 17 lines · Jeremy Fitzhardinge
Quoted 16 lines · Andi Kleen
Quoted 3 lines · Jeremy Fitzhardinge
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
Again it not just upsets Xen, keeping mappings to freed pages is wrong generally and violates the x86 (and likely others like PPC) architecture because it can cause illegal caching attribute aliases.
The patch that went into the tree was really not correct -- this bogus optimization should have been unconditionally removed or if you really wanted an ifdef made dependent on !CONFIG_XEN && !CONFIG_AGP (and likely && !CONFIG_DRM && !CONFIG_anything else that uses uncached mappings in memory).
Quoted 4 lines
Quoted 2 lines
You just worked around the obvious failure and leave the non obvious rare corruptions in, which isn't a good strategy.
Well, at least it becomes a known issue and/or placeholder for when Nick
It's hidden now so it causes any obvious failures any more. Just subtle ones which is much worse.
But why not just disable it? It's not critical functionality, just a optimization that unfortunately turned out to be incorrect.
It could be made correct short term by not freeing the pages until vunmap for example.
Quoted 3 lines
does his grand unified vmap manager. I guess a clean workaround would be to add a
CONFIG_XFS_LAZY_UNMAP, and do it at theKconfiglevel... I'll cook up a patch.
This option could only be safely set in architectures that don't care about caching attribute aliases or never remap kernel pages uncached. That's not x86, powerpc, ia64 at a minimum.
I think the only good short term fix is to turn your ifdef CONFIG_XEN into a #if 0
-Andi
Quoted 3 lines · Andi Kleen
It's hidden now so it causes any obvious failures any more. Just subtle ones which is much worse.
I think anything detected by Xen is still classed as "obscure" ;)
Quoted 6 lines
But why not just disable it? It's not critical functionality, just a optimization that unfortunately turned out to be incorrect.
It could be made correct short term by not freeing the pages until vunmap for example.
I think it only ends up holding 64 pages (or is it 64 mappings?), so its not a horrible use of memory. Particularly since it responds to memory pressure callbacks.
Quoted 5 lines
Quoted 4 lines
does his grand unified vmap manager. I guess a clean workaround would be to add a
CONFIG_XFS_LAZY_UNMAP, and do it at theKconfiglevel... I'll cook up a patch.
Quoted 7 lines
This option could only be safely set in architectures that don't care about caching attribute aliases or never remap kernel pages uncached. That's not x86, powerpc, ia64 at a minimum.
I think the only good short term fix is to turn your ifdef
CONFIG_XENinto a #if 0
#if 1, but yes, I see your point.
J
Quoted 18 lines · Andi Kleen
Quoted 17 lines · Jeremy Fitzhardinge
Quoted 16 lines · Andi Kleen
Quoted 3 lines · Jeremy Fitzhardinge
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
Again it not just upsets Xen, keeping mappings to freed pages is wrong generally and violates the x86 (and likely others like PPC) architecture because it can cause illegal caching attribute aliases.
The patch that went into the tree was really not correct -- this bogus optimization should have been unconditionally removed or if you really wanted an ifdef made dependent on !CONFIG_XEN && !CONFIG_AGP (and likely && !CONFIG_DRM && !CONFIG_anything else that uses uncached mappings in memory).
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
You just worked around the obvious failure and leave the non obvious rare corruptions in, which isn't a good strategy.
Well, at least it becomes a known issue and/or placeholder for when Nick
It's hidden now so it causes any obvious failures any more. Just subtle ones which is much worse.
There's no evidence of any problems ever being caused by this code. It's been there for years.
Quoted 2 lines
But why not just disable it? It's not critical functionality, just a optimization that unfortunately turned out to be incorrect.
It is critical functionality for larger machines because vmap()/vunmap() really does suck in terms of scalability.
Quoted 2 lines
It could be made correct short term by not freeing the pages until vunmap for example.
Could vmap()/vunmap() take references to the pages that are mapped? That way delaying the unmap would also delay the freeing of the pages and hence we'd have no problems with the pages being reused before the mapping is torn down. That'd work for Xen even with XFS's lazy unmapping scheme, and would allow Nick's more advanced methods to work as well....
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 22 lines · David Chinner
Quoted 21 lines · Andi Kleen
Quoted 20 lines · Jeremy Fitzhardinge
Quoted 18 lines · Andi Kleen
Quoted 3 lines · Jeremy Fitzhardinge
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
Again it not just upsets Xen, keeping mappings to freed pages is wrong generally and violates the x86 (and likely others like PPC) architecture because it can cause illegal caching attribute aliases.
The patch that went into the tree was really not correct -- this bogus optimization should have been unconditionally removed or if you really wanted an ifdef made dependent on !CONFIG_XEN && !CONFIG_AGP (and likely && !CONFIG_DRM && !CONFIG_anything else that uses uncached mappings in memory).
You just worked around the obvious failure and leave the non obvious rare corruptions in, which isn't a good strategy.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Well, at least it becomes a known issue and/or placeholder for when Nick
It's hidden now so it causes any obvious failures any more. Just subtle ones which is much worse.
There's no evidence of any problems ever being caused by this code.
We had a fairly nasty bug a couple of years ago with such a mapping that took months to track down. Luckily we had help from a hardware debug lab, without that it would have been very near impossible. But just stabilizing the problem was a nightmare.
You will only see problems if the memory you're still mapping will be allocated by someone who turns it into an uncached mapping. And even then it doesn't happen always because the CPU does not always do the necessary prefetches.
Also the corruption will not be on the XFS side, but on the uncached mapping so typically some data sent to some device gets a few corrupted cache line sized blocks. Depending on the device this might also not be fatal -- e.g. if it is texture data some corrupted pixels occasionally are not that visible. But there can be still cases where it can cause real failures when it hits something critical (the failures were it was tracked down was usually it hitting some command ring and the hardware erroring out)
Also every time I broke change_page_attr() flushing (which handles exactly this problem and is tricky so it got broken a few times) I eventually got complaints/reports from some users who ran into such data inconsistencies. Given it was mostly from closed 3d driver users who seem to be most aggressive in using uncached mappings. But with more systems running more aggressive 3d drivers it'll likely get worse.
Quoted 1 line
It's been there for years.
That doesn't mean it is correct.
Basically you're saying: I don't care if I corrupt device driver data. That's not a very nice thing to say.
Quoted 5 lines
Quoted 2 lines
But why not just disable it? It's not critical functionality, just a optimization that unfortunately turned out to be incorrect.
It is critical functionality for larger machines because vmap()/vunmap() really does suck in terms of scalability.
Critical perhaps, but also broken.
And if it's that big a problem would it be really that difficult to change only the time critical paths using it to not? I assume it's only the directory code where it is a problem? That would be likely even faster in general.
Quoted 9 lines
Quoted 2 lines
It could be made correct short term by not freeing the pages until vunmap for example.
Could vmap()/vunmap() take references to the pages that are mapped? That way delaying the unmap would also delay the freeing of the pages and hence we'd have no problems with the pages being reused before the mapping is torn down. That'd work for Xen even with XFS's lazy unmapping scheme, and would allow Nick's more advanced methods to work as well....
You could always just keep around an array of the pages and then drop the reference count after unmap. Or walk the vmalloc mapping and generate such an array before freeing, then unmap and then drop the reference counts.
Or the other alternative would be change the time critical code that uses it to not.
-Andi
Quoted 6 lines · Andi Kleen
Quoted 5 lines · David Chinner
Quoted 4 lines · Andi Kleen
Quoted 3 lines · Jeremy Fitzhardinge
Quoted 2 lines · Andi Kleen
Jeremy Fitzhardinge <[email protected]> writes:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Yes, that's precisely the problem. xfs does delay the unmap, leaving stray mappings, which upsets Xen.
Again it not just upsets Xen, keeping mappings to freed pages is wrong generally and violates the x86 (and likely others like PPC) architecture because it can cause illegal caching attribute aliases.
It is a serious offense to leave stray mappings for memory which can get re-mapped to I/O devices... especially with PCI and other device hotplug. I have to back up Andi on this one unconditionally.
On architectures where you have multibyte, non-wordsize updates to hardware page tables, you even have races here when setting, updating and clearing PTEs that must be done in a sequence where no aliasing of mappings to partially written PTEs can result in I/O memory getting mapped in a cacheable state. The window here is only one instruction, and yes, it is possible for a window this small to create a problem. A large (or permanently lazy) window is extremely frightening.
These things do cause bugs. The bugs take a very long time to show up and are very difficult to track down, since they can basically cause random behavior, such as hanging the machine or losing orbit and crashing into the moon.
Zach
Quoted 6 lines · Andi Kleen
Quoted 5 lines · David Chinner
Quoted 2 lines · Andi Kleen
It's hidden now so it causes any obvious failures any more. Just subtle ones which is much worse.
There's no evidence of any problems ever being caused by this code.
.....
Quoted 7 lines
Also the corruption will not be on the XFS side, but on the uncached mapping so typically some data sent to some device gets a few corrupted cache line sized blocks. Depending on the device this might also not be fatal -- e.g. if it is texture data some corrupted pixels occasionally are not that visible. But there can be still cases where it can cause real failures when it hits something critical (the failures were it was tracked down was usually it hitting some command ring and the hardware erroring out)
There seems to be little evidence of that occurring around the place.
Quoted 3 lines
Quoted 1 line
It's been there for years.
That doesn't mean it is correct.
Right, but it also points to the fact that it's not causing problems from 99.999% of ppl out there.
Quoted 2 lines
Basically you're saying: I don't care if I corrupt device driver data. That's not a very nice thing to say.
No, I did not say that. I said that there's no evidence that points to this causing problems anywhere.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
But why not just disable it? It's not critical functionality, just a optimization that unfortunately turned out to be incorrect.
It is critical functionality for larger machines because vmap()/vunmap() really does suck in terms of scalability.
Quoted 6 lines
Critical perhaps, but also broken.
And if it's that big a problem would it be really that difficult to change only the time critical paths using it to not? I assume it's only the directory code where it is a problem? That would be likely even faster in general.
Difficult - yes. All the btree code in XFS would have to be rewritten to remove the assumption that the buffer structures are contiguous in memory. Any bug we introduce in doing this will result in on disk corruption, which is far worse than occasionally crashing a machine with a stray mapping.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
It could be made correct short term by not freeing the pages until vunmap for example.
Could vmap()/vunmap() take references to the pages that are mapped? That way delaying the unmap would also delay the freeing of the pages and hence we'd have no problems with the pages being reused before the mapping is torn down. That'd work for Xen even with XFS's lazy unmapping scheme, and would allow Nick's more advanced methods to work as well....
Quoted 3 lines
You could always just keep around an array of the pages and then drop the reference count after unmap. Or walk the vmalloc mapping and generate such an array before freeing, then unmap and then drop the reference counts.
You mean like vmap() could record the pages passed to it in the area->pages array, and we walk and release than in __vunmap() like it already does for vfree()?
If we did this, it would probably be best to pass a page release function into the vmap or vunmap call - we'd need page_cache_release() called on the page rather than __free_page()....
The solution belongs behind the vmap/vunmap interface, not in XFS....
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 16 lines · David Chinner
Quoted 10 lines · Andi Kleen
Quoted 5 lines · David Chinner
Could vmap()/vunmap() take references to the pages that are mapped? That way delaying the unmap would also delay the freeing of the pages and hence we'd have no problems with the pages being reused before the mapping is torn down. That'd work for Xen even with XFS's lazy unmapping scheme, and would allow Nick's more advanced methods to work as well....
You could always just keep around an array of the pages and then drop the reference count after unmap. Or walk the vmalloc mapping and generate such an array before freeing, then unmap and then drop the reference counts.
You mean like
vmap()could record the pages passed to it in thearea->pagesarray, and we walk and release than in__vunmap()like it already does forvfree()?
Quoted 5 lines
If we did this, it would probably be best to pass a page release function into the vmap or vunmap call - we'd need
page_cache_release()called on the page rather than__free_page()....The solution belongs behind the vmap/vunmap interface, not in XFS....
Lightly tested(*) patch that does this with lazy unmapping below for comment.
(*) a) kernel boots, b) made an XFS filesystem with 64k directory blocks, created ~100,000 files in a directory to get a wide btree (~1700 blocks, still only a single level) and run repeated finds across it dropping caches in between. Each traversal maps and unmaps every btree block.
Cheers,
Dave.
Signature · 295 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/linux-2.6/xfs_buf.c | 21 +----
include/linux/vmalloc.h | 4 +
mm/vmalloc.c | 160 +++++++++++++++++++++++++++++++++++----------
3 files changed, 133 insertions(+), 52 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_buf.c 2007-10-18 17:11:06.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_buf.c 2007-10-23 14:48:32.131088616 +1000
@@ -187,19 +187,6 @@ free_address(
{
a_list_t *aentry;
-#ifdef CONFIG_XEN
- /*
- * Xen needs to be able to make sure it can get an exclusive
- * RO mapping of pages it wants to turn into a pagetable. If
- * a newly allocated page is also still being vmap()ed by xfs,
- * it will cause pagetable construction to fail. This is a
- * quick workaround to always eagerly unmap pages so that Xen
- * is happy.
- */
- vunmap(addr);
- return;
-#endif
-
aentry = kmalloc(sizeof(a_list_t), GFP_NOWAIT);
if (likely(aentry)) {
spin_lock(&as_lock);
@@ -209,7 +196,7 @@ free_address(
as_list_len++;
spin_unlock(&as_lock);
} else {
- vunmap(addr);
+ vunmap_pages(addr, put_page);
}
}
@@ -228,7 +215,7 @@ purge_addresses(void)
spin_unlock(&as_lock);
while ((old = aentry) != NULL) {
- vunmap(aentry->vm_addr);
+ vunmap_pages(aentry->vm_addr, put_page);
aentry = aentry->next;
kfree(old);
}
@@ -456,8 +443,8 @@ _xfs_buf_map_pages(
} else if (flags & XBF_MAPPED) {
if (as_list_len > 64)
purge_addresses();
- bp->b_addr = vmap(bp->b_pages, bp->b_page_count,
- VM_MAP, PAGE_KERNEL);
+ bp->b_addr = vmap_pages(bp->b_pages, bp->b_page_count,
+ VM_MAP, PAGE_KERNEL, xb_to_gfp(flags));
if (unlikely(bp->b_addr == NULL))
return -ENOMEM;
bp->b_addr += bp->b_offset;
Index: 2.6.x-xfs-new/include/linux/vmalloc.h
===================================================================
--- 2.6.x-xfs-new.orig/include/linux/vmalloc.h 2007-09-12 15:41:41.000000000 +1000
+++ 2.6.x-xfs-new/include/linux/vmalloc.h 2007-10-23 14:36:56.264860921 +1000
@@ -51,6 +51,10 @@ extern void *vmap(struct page **pages, u
unsigned long flags, pgprot_t prot);
extern void vunmap(void *addr);
+extern void *vmap_pages(struct page **pages, unsigned int count,
+ unsigned long flags, pgprot_t prot, gfp_t gfp_mask);
+extern void vunmap_pages(void *addr, void (*release)(struct page *page));
+
extern int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
unsigned long pgoff);
void vmalloc_sync_all(void);
Index: 2.6.x-xfs-new/mm/vmalloc.c
===================================================================
--- 2.6.x-xfs-new.orig/mm/vmalloc.c 2007-09-12 15:41:48.000000000 +1000
+++ 2.6.x-xfs-new/mm/vmalloc.c 2007-10-23 16:29:40.130384957 +1000
@@ -318,7 +318,56 @@ struct vm_struct *remove_vm_area(void *a
return v;
}
-static void __vunmap(void *addr, int deallocate_pages)
+static void vm_area_release_page(struct page *page)
+{
+ __free_page(page);
+}
+
+static int vm_area_alloc_pagearray(struct vm_struct *area, gfp_t gfp_mask,
+ unsigned int nr_pages, int node)
+{
+ struct page **pages;
+ unsigned int array_size;
+
+ array_size = (nr_pages * sizeof(struct page *));
+
+ area->nr_pages = nr_pages;
+ /* Please note that the recursion is strictly bounded. */
+ if (array_size > PAGE_SIZE) {
+ pages = __vmalloc_node(array_size, gfp_mask | __GFP_ZERO,
+ PAGE_KERNEL, node);
+ area->flags |= VM_VPAGES;
+ } else {
+ pages = kmalloc_node(array_size,
+ (gfp_mask & GFP_LEVEL_MASK) | __GFP_ZERO,
+ node);
+ }
+ area->pages = pages;
+ if (!area->pages) {
+ remove_vm_area(area->addr);
+ kfree(area);
+ return -ENOMEM;
+ }
+ return 0;
+}
+
+static void vm_area_release_pagearray(struct vm_struct *area,
+ void (*release)(struct page *))
+{
+ int i;
+
+ for (i = 0; i < area->nr_pages; i++) {
+ BUG_ON(!area->pages[i]);
+ release(area->pages[i]);
+ }
+
+ if (area->flags & VM_VPAGES)
+ vfree(area->pages);
+ else
+ kfree(area->pages);
+}
+
+static void __vunmap(void *addr, void (*release)(struct page *))
{
struct vm_struct *area;
@@ -341,19 +390,8 @@ static void __vunmap(void *addr, int dea
debug_check_no_locks_freed(addr, area->size);
- if (deallocate_pages) {
- int i;
-
- for (i = 0; i < area->nr_pages; i++) {
- BUG_ON(!area->pages[i]);
- __free_page(area->pages[i]);
- }
-
- if (area->flags & VM_VPAGES)
- vfree(area->pages);
- else
- kfree(area->pages);
- }
+ if (release)
+ vm_area_release_pagearray(area, release);
kfree(area);
return;
@@ -372,7 +410,7 @@ static void __vunmap(void *addr, int dea
void vfree(void *addr)
{
BUG_ON(in_interrupt());
- __vunmap(addr, 1);
+ __vunmap(addr, vm_area_release_page);
}
EXPORT_SYMBOL(vfree);
@@ -388,11 +426,30 @@ EXPORT_SYMBOL(vfree);
void vunmap(void *addr)
{
BUG_ON(in_interrupt());
- __vunmap(addr, 0);
+ __vunmap(addr, NULL);
}
EXPORT_SYMBOL(vunmap);
/**
+ * vunmap_pages - release virtual mapping obtained by vmap_pages()
+ * @addr: memory base address
+ * @release: release page callback function
+ *
+ * Free the virtually contiguous memory area starting at @addr,
+ * which was created from the page array passed to vmap_pages(),
+ * then call the release function on each page in the associated
+ * array of page attached to the area.
+ *
+ * Must not be called in interrupt context.
+ */
+void vunmap_pages(void *addr, void (*release)(struct page *))
+{
+ BUG_ON(in_interrupt());
+ __vunmap(addr, release);
+}
+EXPORT_SYMBOL(vunmap_pages);
+
+/**
* vmap - map an array of pages into virtually contiguous space
* @pages: array of page pointers
* @count: number of pages to map
@@ -422,32 +479,63 @@ void *vmap(struct page **pages, unsigned
}
EXPORT_SYMBOL(vmap);
+/**
+ * vmap_pages - map an array of pages into virtually contiguous space
+ * @pages: array of page pointers
+ * @count: number of pages to map
+ * @flags: vm_area->flags
+ * @prot: page protection for the mapping
+ * @gfp_mask: flags for the page level allocator
+ *
+ * Maps @count pages from @pages into contiguous kernel virtual
+ * space taking a reference to each page and keeping track of all
+ * the pages within the vm area structure.
+ */
+void *vmap_pages(struct page **pages, unsigned int count,
+ unsigned long flags, pgprot_t prot, gfp_t gfp_mask)
+{
+ struct vm_struct *area;
+ struct page **pgp;
+ int i;
+
+ if (count > num_physpages)
+ return NULL;
+
+ area = get_vm_area((count << PAGE_SHIFT), flags);
+ if (!area)
+ return NULL;
+ if (vm_area_alloc_pagearray(area, gfp_mask, count, -1))
+ return NULL;
+
+ /* map_vm_area modifies pgp */
+ pgp = pages;
+ if (map_vm_area(area, prot, &pgp)) {
+ vunmap(area->addr);
+ return NULL;
+ }
+ /*
+ * now that the region is mapped, take a reference to each
+ * page and store them in the area page array.
+ */
+ for (i = 0; i < area->nr_pages; i++) {
+ get_page(pages[i]);
+ area->pages[i] = pages[i];
+ }
+
+ return area->addr;
+}
+EXPORT_SYMBOL(vmap_pages);
+
void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
pgprot_t prot, int node)
{
struct page **pages;
- unsigned int nr_pages, array_size, i;
+ unsigned int nr_pages;
+ int i;
nr_pages = (area->size - PAGE_SIZE) >> PAGE_SHIFT;
- array_size = (nr_pages * sizeof(struct page *));
-
- area->nr_pages = nr_pages;
- /* Please note that the recursion is strictly bounded. */
- if (array_size > PAGE_SIZE) {
- pages = __vmalloc_node(array_size, gfp_mask | __GFP_ZERO,
- PAGE_KERNEL, node);
- area->flags |= VM_VPAGES;
- } else {
- pages = kmalloc_node(array_size,
- (gfp_mask & GFP_LEVEL_MASK) | __GFP_ZERO,
- node);
- }
- area->pages = pages;
- if (!area->pages) {
- remove_vm_area(area->addr);
- kfree(area);
+ if (vm_area_alloc_pagearray(area, gfp_mask, nr_pages, node))
return NULL;
- }
for (i = 0; i < area->nr_pages; i++) {
if (node < 0)
@@ -461,6 +549,8 @@ void *__vmalloc_area_node(struct vm_stru
}
}
+ /* map_vm_area modifies pages */
+ pages = area->pages;
if (map_vm_area(area, prot, &pages))
goto fail;
return area->addr;Quoted 4 lines · David Chinner
Quoted 1 line
That doesn't mean it is correct.
Right, but it also points to the fact that it's not causing problems from 99.999% of ppl out there.
So you're waiting for someone to take months to debug this again?
Quoted 2 lines
You mean like
vmap()could record the pages passed to it in thearea->pagesarray,
The page tables contain pointers to the pages anyways. vunmap() has to walk them. It would not be very difficult to store them in an array during the walk.
Quoted 4 lines
If we did this, it would probably be best to pass a page release function into the vmap or vunmap call - we'd need
page_cache_release()called on the page rather than__free_page()....
Possible. Normally vmalloc pages should not be in the LRU except yours so it would be probably fine to just change it.
Quoted 1 line
The solution belongs behind the vmap/vunmap interface, not in XFS....
You could also just keep the array from map time around yourself. Then you could do it yourself.
-Andi
Quoted 17 lines · David Chinner
Quoted 16 lines · David Chinner
Quoted 10 lines · Andi Kleen
Quoted 5 lines · David Chinner
Could vmap()/vunmap() take references to the pages that are mapped? That way delaying the unmap would also delay the freeing of the pages and hence we'd have no problems with the pages being reused before the mapping is torn down. That'd work for Xen even with XFS's lazy unmapping scheme, and would allow Nick's more advanced methods to work as well....
You could always just keep around an array of the pages and then drop the reference count after unmap. Or walk the vmalloc mapping and generate such an array before freeing, then unmap and then drop the reference counts.
You mean like
vmap()could record the pages passed to it in thearea->pagesarray, and we walk and release than in__vunmap()like it already does forvfree()?
Quoted 8 lines
Quoted 5 lines
If we did this, it would probably be best to pass a page release function into the vmap or vunmap call - we'd need
page_cache_release()called on the page rather than__free_page()....The solution belongs behind the vmap/vunmap interface, not in XFS....
Lightly
tested(*)patch that does this with lazy unmapping below for comment.
Thanks
Quoted 6 lines
(*) a) kernel boots, b) made an XFS filesystem with 64k directory blocks, created ~100,000 files in a directory to get a wide btree (~1700 blocks, still only a single level) and run repeated finds across it dropping caches in between. Each traversal maps and unmaps every btree block.
Hmm, the __free_page -> page_cache_release() change in vfree() would have been simpler wouldn't it?
But if it works it is fine.
-Andi
Quoted 17 lines · Andi Kleen
Quoted 4 lines · David Chinner
Quoted 4 lines
You mean like
vmap()could record the pages passed to it in thearea->pagesarray, and we walk and release than in__vunmap()like it already does forvfree()?Quoted 8 lines
Quoted 5 lines
If we did this, it would probably be best to pass a page release function into the vmap or vunmap call - we'd need
page_cache_release()called on the page rather than__free_page()....The solution belongs behind the vmap/vunmap interface, not in XFS....
Lightly
tested(*)patch that does this with lazy unmapping below for comment.Thanks
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
(*) a) kernel boots, b) made an XFS filesystem with 64k directory blocks, created ~100,000 files in a directory to get a wide btree (~1700 blocks, still only a single level) and run repeated finds across it dropping caches in between. Each traversal maps and unmaps every btree block.
Hmm, the
__free_page->page_cache_release()change invfree()would have been simpler wouldn't it?
Yes, it would, but I tried to leave vmalloc doing the same thing as it was before. I think that it would be safe simply to call put_page() directly in the __vunmap() code and drop all the release function passing, but I don't know enough about that code to say for certain. I'll spin up another patch tomorrow that does this and see how it goes.
Quoted 1 line
But if it works it is fine.
Seems to - it's passed XFSQA with 64k directories and a bunch of dirstress workouts as well.
Nick, Jeremy, (others?) any objections to this approach to solve the problem?
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 2 lines · David Chinner
Nick, Jeremy, (others?) any objections to this approach to solve the problem?
Seems sounds in principle. I think Nick's shootdown-stray-mappings mm
API call is a better long-term answer, but this will do for now. I'll test it out today.
J
PATCH 1/1 Allow lazy unmapping by taking extra page references V2
In reply to Andi KleenAllow lazy unmapping of vmap()d regions be taking a reference on each page in the region being vmap()ed. The page references get released after the region is vunmap()ed, thereby ensuring we don't leave stray mappings on freed pages that could lead to problems pages being reallocated with incorrect mappings associated with them.
Tested with XFS filesystems with 64k directory blocks with dirstress and XFSQA.
Version 2:
- drop the release function stuff and just call put_page() as it falls down to the same code in all calling cases.
--- fs/xfs/linux-2.6/xfs_buf.c | 21 +------- include/linux/vmalloc.h | 4 + mm/vmalloc.c | 118 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 106 insertions(+), 37 deletions(-)
fs/xfs/linux-2.6/xfs_buf.c +4 -17
@@ -187,19 +187,6 @@ free_address( {a_list_t *aentry; -#ifdef CONFIG_XEN -/* -* Xen needs to be able to make sure it can get an exclusive -* RO mapping of pages it wants to turn into a pagetable. If -* a newly allocated page is also still being vmap()ed by xfs, -* it will cause pagetable construction to fail. This is a -* quick workaround to always eagerly unmap pages so that Xen -* is happy. -*/ -vunmap (addr); -return ; -#endif - aentry =kmalloc (sizeof (a_list_t),GFP_NOWAIT );if (likely (aentry)) {spin_lock (&as_lock);
@@ -209,7 +196,7 @@ free_address( as_list_len++;spin_unlock (&as_lock); }else { -vunmap (addr); +vunmap_pages (addr); } }
@@ -228,7 +215,7 @@ purge_addresses(void)spin_unlock (&as_lock);while ((old = aentry) !=NULL ) { -vunmap (aentry->vm_addr); +vunmap_pages (aentry->vm_addr); aentry = aentry->next;kfree (old); }
@@ -456,8 +443,8 @@ _xfs_buf_map_pages( }else if (flags &XBF_MAPPED ) {if (as_list_len >64 )purge_addresses (); - bp->b_addr =vmap (bp->b_pages, bp->b_page_count, -VM_MAP ,PAGE_KERNEL ); + bp->b_addr =vmap_pages (bp->b_pages, bp->b_page_count, +VM_MAP ,PAGE_KERNEL ,xb_to_gfp (flags));if (unlikely (bp->b_addr ==NULL ))return -ENOMEM ; bp->b_addr += bp->b_offset;
include/linux/vmalloc.h +4 -0
@@ -51,6 +51,10 @@ extern void *vmap(struct page **pages, u unsigned long flags, pgprot_t prot);extern void vunmap (void *addr); +extern void *vmap_pages (struct page **pages,unsigned int count, +unsigned long flags,pgprot_t prot,gfp_t gfp_mask); +extern void vunmap_pages (void *addr); +extern int remap_vmalloc_range (struct vm_area_struct *vma,void *addr,unsigned long pgoff);void vmalloc_sync_all (void );
mm/vmalloc.c +98 -20
155 lines not on this page · raw message
Re: [PATCH] Allow lazy unmapping by taking extra page references V2
Quoted 7 lines · David Chinner
Allow lazy unmapping of vmap()d regions be taking a reference on each page in the region being vmap()ed. The page references get released after the region is vunmap()ed, thereby ensuring we don't leave stray mappings on freed pages that could lead to problems pages being reallocated with incorrect mappings associated with them.
Quoted 7 lines
Tested with XFS filesystems with 64k directory blocks with dirstress and XFSQA.
Version 2:
- drop the release function stuff and just callput_page()as it falls down to the same code in all calling cases.
This doesn't apply cleanly to the current git tree.
J
PATCH 1/1 Allow lazy unmapping by taking extra page references V3
Allow lazy unmapping of vmap()d regions be taking a reference on each page in the region being vmap()ed. The page references get released after the region is vunmap()ed, thereby ensuring we don't leave stray mappings on freed pages that could lead to problems pages being reallocated with incorrect mappings associated with them.
Tested with XFS filesystems with 64k directory blocks with dirstress and XFSQA.
Version 3:
- compile on latest -git
Version 2:
- drop the release function stuff and just call put_page() as it falls down to the same code in all calling cases.
---
fs/xfs/linux-2.6/xfs_buf.c | 21 +------- include/linux/vmalloc.h | 4 + mm/vmalloc.c | 118 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 106 insertions(+), 37 deletions(-)
fs/xfs/linux-2.6/xfs_buf.c +4 -17
@@ -187,19 +187,6 @@ free_address( {a_list_t *aentry; -#ifdef CONFIG_XEN -/* -* Xen needs to be able to make sure it can get an exclusive -* RO mapping of pages it wants to turn into a pagetable. If -* a newly allocated page is also still being vmap()ed by xfs, -* it will cause pagetable construction to fail. This is a -* quick workaround to always eagerly unmap pages so that Xen -* is happy. -*/ -vunmap (addr); -return ; -#endif - aentry =kmalloc (sizeof (a_list_t),GFP_NOWAIT );if (likely (aentry)) {spin_lock (&as_lock);
@@ -209,7 +196,7 @@ #endif as_list_len++;spin_unlock (&as_lock); }else { -vunmap (addr); +vunmap_pages (addr); } }
@@ -228,7 +215,7 @@ purge_addresses(void)spin_unlock (&as_lock);while ((old = aentry) !=NULL ) { -vunmap (aentry->vm_addr); +vunmap_pages (aentry->vm_addr); aentry = aentry->next;kfree (old); }
@@ -458,8 +445,8 @@ _xfs_buf_map_pages( }else if (flags &XBF_MAPPED ) {if (as_list_len >64 )purge_addresses (); - bp->b_addr =vmap (bp->b_pages, bp->b_page_count, -VM_MAP ,PAGE_KERNEL ); + bp->b_addr =vmap_pages (bp->b_pages, bp->b_page_count, +VM_MAP ,PAGE_KERNEL ,xb_to_gfp (flags));if (unlikely (bp->b_addr ==NULL ))return -ENOMEM ; bp->b_addr += bp->b_offset;
include/linux/vmalloc.h +4 -0
@@ -51,6 +51,10 @@ extern void *vmap(struct page **pages, u unsigned long flags, pgprot_t prot);extern void vunmap (void *addr); +extern void *vmap_pages (struct page **pages,unsigned int count, +unsigned long flags,pgprot_t prot,gfp_t gfp_mask); +extern void vunmap_pages (void *addr); +extern int remap_vmalloc_range (struct vm_area_struct *vma,void *addr,unsigned long pgoff);void vmalloc_sync_all (void );
mm/vmalloc.c +98 -20
155 lines not on this page · raw message
Re: [PATCH] Allow lazy unmapping by taking extra page references V3
Quoted 7 lines · David Chinner
Allow lazy unmapping of vmap()d regions be taking a reference on each page in the region being vmap()ed. The page references get released after the region is vunmap()ed, thereby ensuring we don't leave stray mappings on freed pages that could lead to problems pages being reallocated with incorrect mappings associated with them.
Quoted 6 lines
Tested with XFS filesystems with 64k directory blocks with dirstress and XFSQA.
Version 3:
- compile on latest -git
Not quite:
CC mm/vmalloc.o/home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c: In function 'vm_area_alloc_pagearray':/home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c:338: error: 'GFP_LEVEL_MASK' undeclared (first use in this function) /home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c:338: error: (Each undeclared identifier is reported only once /home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c:338: error: for each function it appears in.) make[3]: *** [mm/vmalloc.o] Error 1
make[2]: *** [mm] Error 2
make[2]: *** Waiting for unfinished jobs....
GFP_RECLAM_MASK now?
J
Re: [PATCH] Allow lazy unmapping by taking extra page references V3
Quoted 17 lines · Jeremy Fitzhardinge
Quoted 3 lines · David Chinner
Version 3:
- compile on latest -gitNot quite:
CC
mm/vmalloc.o/home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c: In function 'vm_area_alloc_pagearray':/home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c:338: error: 'GFP_LEVEL_MASK' undeclared (first use in this function)/home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c:338: error: (Each undeclared identifier is reported only once/home/jeremy/hg/xen/paravirt/linux/mm/vmalloc.c:338: error: for each function it appears in.) make[3]: *** [mm/vmalloc.o] Error 1
make[2]: *** [mm] Error 2
make[2]: *** Waiting for unfinished jobs....
Quoted 1 line
GFP_RECLAM_MASKnow?
Yeah, it is. Not sure what happened there - I did make that change. new diff below.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
fs/xfs/linux-2.6/xfs_buf.c +4 -17
@@ -187,19 +187,6 @@ free_address( {a_list_t *aentry; -#ifdef CONFIG_XEN -/* -* Xen needs to be able to make sure it can get an exclusive -* RO mapping of pages it wants to turn into a pagetable. If -* a newly allocated page is also still being vmap()ed by xfs, -* it will cause pagetable construction to fail. This is a -* quick workaround to always eagerly unmap pages so that Xen -* is happy. -*/ -vunmap (addr); -return ; -#endif - aentry =kmalloc (sizeof (a_list_t),GFP_NOWAIT );if (likely (aentry)) {spin_lock (&as_lock);
@@ -209,7 +196,7 @@ #endif as_list_len++;spin_unlock (&as_lock); }else { -vunmap (addr); +vunmap_pages (addr); } }
@@ -228,7 +215,7 @@ purge_addresses(void)spin_unlock (&as_lock);while ((old = aentry) !=NULL ) { -vunmap (aentry->vm_addr); +vunmap_pages (aentry->vm_addr); aentry = aentry->next;kfree (old); }
@@ -458,8 +445,8 @@ _xfs_buf_map_pages( }else if (flags &XBF_MAPPED ) {if (as_list_len >64 )purge_addresses (); - bp->b_addr =vmap (bp->b_pages, bp->b_page_count, -VM_MAP ,PAGE_KERNEL ); + bp->b_addr =vmap_pages (bp->b_pages, bp->b_page_count, +VM_MAP ,PAGE_KERNEL ,xb_to_gfp (flags));if (unlikely (bp->b_addr ==NULL ))return -ENOMEM ; bp->b_addr += bp->b_offset;
include/linux/vmalloc.h +4 -0
@@ -51,6 +51,10 @@ extern void *vmap(struct page **pages, u unsigned long flags, pgprot_t prot);extern void vunmap (void *addr); +extern void *vmap_pages (struct page **pages,unsigned int count, +unsigned long flags,pgprot_t prot,gfp_t gfp_mask); +extern void vunmap_pages (void *addr); +extern int remap_vmalloc_range (struct vm_area_struct *vma,void *addr,unsigned long pgoff);void vmalloc_sync_all (void );
mm/vmalloc.c +98 -20
155 lines not on this page · raw message