Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote: > On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: > > > > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > > > > +bool isolate_balloon_page(struct page *page) > > > > > +{ > > > > > + if (WARN_ON(!movable_balloon_page(page))) > > > > > > > > Looks like this actually can happen if the page is leaked > > > > between previous movable_balloon_page and here. > > > > > > > > > + return false; > > > > > > Yes, it surely can happen, and it does not harm to catch it here, print a > > > warn and > > > return. > > > > If it is legal, why warn? For that matter why test here at all? > > > > As this is a public symbol, and despite the usage we introduce is sane, the > warn > was placed as an insurance policy to let us know about any insane attempt to > use > the procedure in the future. That was due to a nice review nitpick, actually. > > Even though the code already had a test to properly avoid this race you > mention, I thought that sustaining the warn was a good thing. As I told you, > despite real, I've never got (un)lucky enough to stumble across that race > window > while testing the patch. > > If your concern is about being too much verbose on logging, under certain > conditions, perhaps we can change that test to a WARN_ON_ONCE() ? > > Mel, what are your thoughts here? > I viewed it as being defensive programming. VM_BUG_ON would be less useful as it can be compiled out. If the race can be routinely hit then multiple warnings is instructive in itself. I have no strong feelings about this though. I see little harm in making the check but in light of this conversation add a short comment explaining that the check should be redundant. -- Mel Gorman SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote: On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. If it is legal, why warn? For that matter why test here at all? As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually. Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch. If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ? Mel, what are your thoughts here? I viewed it as being defensive programming. VM_BUG_ON would be less useful as it can be compiled out. If the race can be routinely hit then multiple warnings is instructive in itself. I have no strong feelings about this though. I see little harm in making the check but in light of this conversation add a short comment explaining that the check should be redundant. -- Mel Gorman SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote: > On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: > > > > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > > > > +bool isolate_balloon_page(struct page *page) > > > > > +{ > > > > > + if (WARN_ON(!movable_balloon_page(page))) > > > > > > > > Looks like this actually can happen if the page is leaked > > > > between previous movable_balloon_page and here. > > > > > > > > > + return false; > > > > > > Yes, it surely can happen, and it does not harm to catch it here, print a > > > warn and > > > return. > > > > If it is legal, why warn? For that matter why test here at all? > > > > As this is a public symbol, and despite the usage we introduce is sane, the > warn > was placed as an insurance policy to let us know about any insane attempt to > use > the procedure in the future. That was due to a nice review nitpick, actually. > > Even though the code already had a test to properly avoid this race you > mention, I thought that sustaining the warn was a good thing. As I told you, > despite real, I've never got (un)lucky enough to stumble across that race > window > while testing the patch. > > If your concern is about being too much verbose on logging, under certain > conditions, perhaps we can change that test to a WARN_ON_ONCE() ? > > Mel, what are your thoughts here? > > > > While testing it, I wasn't lucky to see this small window opening, though. think about it: you see it in log. so you know race triggered. now what? why is it useful to know this? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 10:48:37PM +0300, Michael S. Tsirkin wrote: > > > > E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. > > This is mm stuff it is best not to tie it to specific drivers. > > But of course I agree this is not top priority, no need > to block submission on this, just nice to have. > This surely is interesting to look at, in the near future. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: > > > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > > > +bool isolate_balloon_page(struct page *page) > > > > +{ > > > > + if (WARN_ON(!movable_balloon_page(page))) > > > > > > Looks like this actually can happen if the page is leaked > > > between previous movable_balloon_page and here. > > > > > > > + return false; > > > > Yes, it surely can happen, and it does not harm to catch it here, print a > > warn and > > return. > > If it is legal, why warn? For that matter why test here at all? > As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually. Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch. If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ? Mel, what are your thoughts here? > > While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote: > On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: > > > +static inline bool movable_balloon_page(struct page *page) > > > +{ > > > + return (page->mapping && page->mapping == balloon_mapping); > > > > I am guessing this needs smp_read_barrier_depends, and maybe > > ACCESS_ONCE ... > > > > I'm curious about your guessing here. Could you ellaborate it further, please? well balloon_mapping can change dynamically. I think actually rcu is a good fit here. > > > > +#else > > > +static inline bool isolate_balloon_page(struct page *page) { return > > > false; } > > > +static inline void putback_balloon_page(struct page *page) { return > > > false; } > > > +static inline bool movable_balloon_page(struct page *page) { return > > > false; } > > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION > > > */ > > > + > > > > This does mean that only one type of balloon is useable at a time. > > I wonder whether using a flag in address_space structure instead > > is possible ... > > This means we are only introducing this feature for virtio_balloon by now. > Despite the flagging address_space stuff is something we surely can look in > the > future, I quite didn't get how we could be using two different types of > balloon > devices at the same time for the same system. Could you ellaborate it a little > more, please? > > > > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > > +bool isolate_balloon_page(struct page *page) > > > +{ > > > + if (WARN_ON(!movable_balloon_page(page))) > > > > Looks like this actually can happen if the page is leaked > > between previous movable_balloon_page and here. > > > > > + return false; > > Yes, it surely can happen, and it does not harm to catch it here, print a > warn and > return. While testing it, I wasn't lucky to see this small window opening, > though. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: > On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote: > > On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: > > > > +static inline bool movable_balloon_page(struct page *page) > > > > +{ > > > > + return (page->mapping && page->mapping == balloon_mapping); > > > > > > I am guessing this needs smp_read_barrier_depends, and maybe > > > ACCESS_ONCE ... > > > > > > > I'm curious about your guessing here. Could you ellaborate it further, > > please? > > > > > > > > +#else > > > > +static inline bool isolate_balloon_page(struct page *page) { return > > > > false; } > > > > +static inline void putback_balloon_page(struct page *page) { return > > > > false; } > > > > +static inline bool movable_balloon_page(struct page *page) { return > > > > false; } > > > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && > > > > CONFIG_COMPACTION */ > > > > + > > > > > > This does mean that only one type of balloon is useable at a time. > > > I wonder whether using a flag in address_space structure instead > > > is possible ... > > > > This means we are only introducing this feature for virtio_balloon by now. > > Despite the flagging address_space stuff is something we surely can look in > > the > > future, I quite didn't get how we could be using two different types of > > balloon > > devices at the same time for the same system. Could you ellaborate it a > > little > > more, please? > > > > E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. > This is mm stuff it is best not to tie it to specific drivers. But of course I agree this is not top priority, no need to block submission on this, just nice to have. > > > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > > > +bool isolate_balloon_page(struct page *page) > > > > +{ > > > > + if (WARN_ON(!movable_balloon_page(page))) > > > > > > Looks like this actually can happen if the page is leaked > > > between previous movable_balloon_page and here. > > > > > > > + return false; > > > > Yes, it surely can happen, and it does not harm to catch it here, print a > > warn and > > return. > > If it is legal, why warn? For that matter why test here at all? > > > While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote: > On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: > > > +static inline bool movable_balloon_page(struct page *page) > > > +{ > > > + return (page->mapping && page->mapping == balloon_mapping); > > > > I am guessing this needs smp_read_barrier_depends, and maybe > > ACCESS_ONCE ... > > > > I'm curious about your guessing here. Could you ellaborate it further, please? > > > > > +#else > > > +static inline bool isolate_balloon_page(struct page *page) { return > > > false; } > > > +static inline void putback_balloon_page(struct page *page) { return > > > false; } > > > +static inline bool movable_balloon_page(struct page *page) { return > > > false; } > > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION > > > */ > > > + > > > > This does mean that only one type of balloon is useable at a time. > > I wonder whether using a flag in address_space structure instead > > is possible ... > > This means we are only introducing this feature for virtio_balloon by now. > Despite the flagging address_space stuff is something we surely can look in > the > future, I quite didn't get how we could be using two different types of > balloon > devices at the same time for the same system. Could you ellaborate it a little > more, please? > E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers. > > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > > +bool isolate_balloon_page(struct page *page) > > > +{ > > > + if (WARN_ON(!movable_balloon_page(page))) > > > > Looks like this actually can happen if the page is leaked > > between previous movable_balloon_page and here. > > > > > + return false; > > Yes, it surely can happen, and it does not harm to catch it here, print a > warn and > return. If it is legal, why warn? For that matter why test here at all? > While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: > > +static inline bool movable_balloon_page(struct page *page) > > +{ > > + return (page->mapping && page->mapping == balloon_mapping); > > I am guessing this needs smp_read_barrier_depends, and maybe > ACCESS_ONCE ... > I'm curious about your guessing here. Could you ellaborate it further, please? > > +#else > > +static inline bool isolate_balloon_page(struct page *page) { return false; > > } > > +static inline void putback_balloon_page(struct page *page) { return false; > > } > > +static inline bool movable_balloon_page(struct page *page) { return false; > > } > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */ > > + > > This does mean that only one type of balloon is useable at a time. > I wonder whether using a flag in address_space structure instead > is possible ... This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please? > > +/* __isolate_lru_page() counterpart for a ballooned page */ > > +bool isolate_balloon_page(struct page *page) > > +{ > > + if (WARN_ON(!movable_balloon_page(page))) > > Looks like this actually can happen if the page is leaked > between previous movable_balloon_page and here. > > > + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: +static inline bool movable_balloon_page(struct page *page) +{ + return (page-mapping page-mapping == balloon_mapping); I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ... I'm curious about your guessing here. Could you ellaborate it further, please? +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) CONFIG_COMPACTION */ + This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ... This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please? +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote: On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: +static inline bool movable_balloon_page(struct page *page) +{ + return (page-mapping page-mapping == balloon_mapping); I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ... I'm curious about your guessing here. Could you ellaborate it further, please? +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) CONFIG_COMPACTION */ + This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ... This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please? E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers. +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. If it is legal, why warn? For that matter why test here at all? While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote: On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: +static inline bool movable_balloon_page(struct page *page) +{ + return (page-mapping page-mapping == balloon_mapping); I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ... I'm curious about your guessing here. Could you ellaborate it further, please? +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) CONFIG_COMPACTION */ + This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ... This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please? E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers. But of course I agree this is not top priority, no need to block submission on this, just nice to have. +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. If it is legal, why warn? For that matter why test here at all? While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote: On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote: +static inline bool movable_balloon_page(struct page *page) +{ + return (page-mapping page-mapping == balloon_mapping); I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ... I'm curious about your guessing here. Could you ellaborate it further, please? well balloon_mapping can change dynamically. I think actually rcu is a good fit here. +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) CONFIG_COMPACTION */ + This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ... This means we are only introducing this feature for virtio_balloon by now. Despite the flagging address_space stuff is something we surely can look in the future, I quite didn't get how we could be using two different types of balloon devices at the same time for the same system. Could you ellaborate it a little more, please? +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. If it is legal, why warn? For that matter why test here at all? As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually. Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch. If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ? Mel, what are your thoughts here? While testing it, I wasn't lucky to see this small window opening, though. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 10:48:37PM +0300, Michael S. Tsirkin wrote: E.g. kvm can emulate hyperv so it could in theory have hyperv balloon. This is mm stuff it is best not to tie it to specific drivers. But of course I agree this is not top priority, no need to block submission on this, just nice to have. This surely is interesting to look at, in the near future. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote: On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote: +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; Yes, it surely can happen, and it does not harm to catch it here, print a warn and return. If it is legal, why warn? For that matter why test here at all? As this is a public symbol, and despite the usage we introduce is sane, the warn was placed as an insurance policy to let us know about any insane attempt to use the procedure in the future. That was due to a nice review nitpick, actually. Even though the code already had a test to properly avoid this race you mention, I thought that sustaining the warn was a good thing. As I told you, despite real, I've never got (un)lucky enough to stumble across that race window while testing the patch. If your concern is about being too much verbose on logging, under certain conditions, perhaps we can change that test to a WARN_ON_ONCE() ? Mel, what are your thoughts here? While testing it, I wasn't lucky to see this small window opening, though. think about it: you see it in log. so you know race triggered. now what? why is it useful to know this? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Fri, Aug 10, 2012 at 02:55:14PM -0300, Rafael Aquini wrote: > Memory fragmentation introduced by ballooning might reduce significantly > the number of 2MB contiguous memory blocks that can be used within a guest, > thus imposing performance penalties associated with the reduced number of > transparent huge pages that could be used by the guest workload. > > This patch introduces the helper functions as well as the necessary changes > to teach compaction and migration bits how to cope with pages which are > part of a guest memory balloon, in order to make them movable by memory > compaction procedures. > > Signed-off-by: Rafael Aquini > --- > include/linux/mm.h | 17 > mm/compaction.c| 125 > + > mm/migrate.c | 30 - > 3 files changed, 152 insertions(+), 20 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 311be90..56cc553 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1662,5 +1662,22 @@ static inline unsigned int > debug_guardpage_minorder(void) { return 0; } > static inline bool page_is_guard(struct page *page) { return false; } > #endif /* CONFIG_DEBUG_PAGEALLOC */ > > +#if (defined(CONFIG_VIRTIO_BALLOON) || \ > + defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION) > +extern bool isolate_balloon_page(struct page *); > +extern void putback_balloon_page(struct page *); > +extern struct address_space *balloon_mapping; > + > +static inline bool movable_balloon_page(struct page *page) > +{ > + return (page->mapping && page->mapping == balloon_mapping); I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ... > +} > + > +#else > +static inline bool isolate_balloon_page(struct page *page) { return false; } > +static inline void putback_balloon_page(struct page *page) { return false; } > +static inline bool movable_balloon_page(struct page *page) { return false; } > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */ > + This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ... > #endif /* __KERNEL__ */ > #endif /* _LINUX_MM_H */ > diff --git a/mm/compaction.c b/mm/compaction.c > index e78cb96..e4e871b 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include "internal.h" > > #if defined CONFIG_COMPACTION || defined CONFIG_CMA > @@ -21,6 +22,84 @@ > #define CREATE_TRACE_POINTS > #include > > +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE) > +/* > + * Balloon pages special page->mapping. > + * Users must properly allocate and initialize an instance of > balloon_mapping, > + * and set it as the page->mapping for balloon enlisted page instances. > + * There is no need on utilizing struct address_space locking schemes for > + * balloon_mapping as, once it gets initialized at balloon driver, it will > + * remain just like a static reference that helps us on identifying a guest > + * ballooned page by its mapping, as well as it will keep the 'a_ops' > callback > + * pointers to the functions that will execute the balloon page mobility > tasks. > + * > + * address_space_operations necessary methods for ballooned pages: > + * .migratepage- used to perform balloon's page migration (as is) > + * .invalidatepage - used to isolate a page from balloon's page list > + * .freepage - used to reinsert an isolated page to balloon's page > list > + */ > +struct address_space *balloon_mapping; > +EXPORT_SYMBOL_GPL(balloon_mapping); > + > +static inline void __isolate_balloon_page(struct page *page) > +{ > + page->mapping->a_ops->invalidatepage(page, 0); > +} > + > +static inline void __putback_balloon_page(struct page *page) > +{ > + page->mapping->a_ops->freepage(page); > +} > + > +/* __isolate_lru_page() counterpart for a ballooned page */ > +bool isolate_balloon_page(struct page *page) > +{ > + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. > + return false; > + > + if (likely(get_page_unless_zero(page))) { > + /* > + * As balloon pages are not isolated from LRU lists, concurrent > + * compaction threads can race against page migration functions > + * move_to_new_page() & __unmap_and_move(). > + * In order to avoid having an already isolated balloon page > + * being (wrongly) re-isolated while it is under migration, > + * lets be sure we have the page lock before proceeding with > + * the balloon page isolation steps. > + */ > + if (likely(trylock_page(page))) { > + /* > + * A ballooned
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Fri, Aug 10, 2012 at 02:55:14PM -0300, Rafael Aquini wrote: Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload. This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures. Signed-off-by: Rafael Aquini aqu...@redhat.com --- include/linux/mm.h | 17 mm/compaction.c| 125 + mm/migrate.c | 30 - 3 files changed, 152 insertions(+), 20 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index 311be90..56cc553 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1662,5 +1662,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; } static inline bool page_is_guard(struct page *page) { return false; } #endif /* CONFIG_DEBUG_PAGEALLOC */ +#if (defined(CONFIG_VIRTIO_BALLOON) || \ + defined(CONFIG_VIRTIO_BALLOON_MODULE)) defined(CONFIG_COMPACTION) +extern bool isolate_balloon_page(struct page *); +extern void putback_balloon_page(struct page *); +extern struct address_space *balloon_mapping; + +static inline bool movable_balloon_page(struct page *page) +{ + return (page-mapping page-mapping == balloon_mapping); I am guessing this needs smp_read_barrier_depends, and maybe ACCESS_ONCE ... +} + +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) CONFIG_COMPACTION */ + This does mean that only one type of balloon is useable at a time. I wonder whether using a flag in address_space structure instead is possible ... #endif /* __KERNEL__ */ #endif /* _LINUX_MM_H */ diff --git a/mm/compaction.c b/mm/compaction.c index e78cb96..e4e871b 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -14,6 +14,7 @@ #include linux/backing-dev.h #include linux/sysctl.h #include linux/sysfs.h +#include linux/export.h #include internal.h #if defined CONFIG_COMPACTION || defined CONFIG_CMA @@ -21,6 +22,84 @@ #define CREATE_TRACE_POINTS #include trace/events/compaction.h +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE) +/* + * Balloon pages special page-mapping. + * Users must properly allocate and initialize an instance of balloon_mapping, + * and set it as the page-mapping for balloon enlisted page instances. + * There is no need on utilizing struct address_space locking schemes for + * balloon_mapping as, once it gets initialized at balloon driver, it will + * remain just like a static reference that helps us on identifying a guest + * ballooned page by its mapping, as well as it will keep the 'a_ops' callback + * pointers to the functions that will execute the balloon page mobility tasks. + * + * address_space_operations necessary methods for ballooned pages: + * .migratepage- used to perform balloon's page migration (as is) + * .invalidatepage - used to isolate a page from balloon's page list + * .freepage - used to reinsert an isolated page to balloon's page list + */ +struct address_space *balloon_mapping; +EXPORT_SYMBOL_GPL(balloon_mapping); + +static inline void __isolate_balloon_page(struct page *page) +{ + page-mapping-a_ops-invalidatepage(page, 0); +} + +static inline void __putback_balloon_page(struct page *page) +{ + page-mapping-a_ops-freepage(page); +} + +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) Looks like this actually can happen if the page is leaked between previous movable_balloon_page and here. + return false; + + if (likely(get_page_unless_zero(page))) { + /* + * As balloon pages are not isolated from LRU lists, concurrent + * compaction threads can race against page migration functions + * move_to_new_page() __unmap_and_move(). + * In order to avoid having an already isolated balloon page + * being (wrongly) re-isolated while it is under migration, + * lets be sure we have the page lock before proceeding with + * the balloon page isolation steps. + */ + if (likely(trylock_page(page))) { + /* + * A ballooned page, by default, has just one
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Fri, Aug 10, 2012 at 02:55:14PM -0300, Rafael Aquini wrote: > Memory fragmentation introduced by ballooning might reduce significantly > the number of 2MB contiguous memory blocks that can be used within a guest, > thus imposing performance penalties associated with the reduced number of > transparent huge pages that could be used by the guest workload. > > This patch introduces the helper functions as well as the necessary changes > to teach compaction and migration bits how to cope with pages which are > part of a guest memory balloon, in order to make them movable by memory > compaction procedures. > > Signed-off-by: Rafael Aquini Reviewed-by: Minchan Kim -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
On Fri, Aug 10, 2012 at 02:55:14PM -0300, Rafael Aquini wrote: Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload. This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures. Signed-off-by: Rafael Aquini aqu...@redhat.com Reviewed-by: Minchan Kim minc...@kernel.org -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload. This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures. Signed-off-by: Rafael Aquini --- include/linux/mm.h | 17 mm/compaction.c| 125 + mm/migrate.c | 30 - 3 files changed, 152 insertions(+), 20 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index 311be90..56cc553 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1662,5 +1662,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; } static inline bool page_is_guard(struct page *page) { return false; } #endif /* CONFIG_DEBUG_PAGEALLOC */ +#if (defined(CONFIG_VIRTIO_BALLOON) || \ + defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION) +extern bool isolate_balloon_page(struct page *); +extern void putback_balloon_page(struct page *); +extern struct address_space *balloon_mapping; + +static inline bool movable_balloon_page(struct page *page) +{ + return (page->mapping && page->mapping == balloon_mapping); +} + +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */ + #endif /* __KERNEL__ */ #endif /* _LINUX_MM_H */ diff --git a/mm/compaction.c b/mm/compaction.c index e78cb96..e4e871b 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -14,6 +14,7 @@ #include #include #include +#include #include "internal.h" #if defined CONFIG_COMPACTION || defined CONFIG_CMA @@ -21,6 +22,84 @@ #define CREATE_TRACE_POINTS #include +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE) +/* + * Balloon pages special page->mapping. + * Users must properly allocate and initialize an instance of balloon_mapping, + * and set it as the page->mapping for balloon enlisted page instances. + * There is no need on utilizing struct address_space locking schemes for + * balloon_mapping as, once it gets initialized at balloon driver, it will + * remain just like a static reference that helps us on identifying a guest + * ballooned page by its mapping, as well as it will keep the 'a_ops' callback + * pointers to the functions that will execute the balloon page mobility tasks. + * + * address_space_operations necessary methods for ballooned pages: + * .migratepage- used to perform balloon's page migration (as is) + * .invalidatepage - used to isolate a page from balloon's page list + * .freepage - used to reinsert an isolated page to balloon's page list + */ +struct address_space *balloon_mapping; +EXPORT_SYMBOL_GPL(balloon_mapping); + +static inline void __isolate_balloon_page(struct page *page) +{ + page->mapping->a_ops->invalidatepage(page, 0); +} + +static inline void __putback_balloon_page(struct page *page) +{ + page->mapping->a_ops->freepage(page); +} + +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) + return false; + + if (likely(get_page_unless_zero(page))) { + /* +* As balloon pages are not isolated from LRU lists, concurrent +* compaction threads can race against page migration functions +* move_to_new_page() & __unmap_and_move(). +* In order to avoid having an already isolated balloon page +* being (wrongly) re-isolated while it is under migration, +* lets be sure we have the page lock before proceeding with +* the balloon page isolation steps. +*/ + if (likely(trylock_page(page))) { + /* +* A ballooned page, by default, has just one refcount. +* Prevent concurrent compaction threads from isolating +* an already isolated balloon page. +*/ + if (movable_balloon_page(page) && + (page_count(page) == 2)) { + __isolate_balloon_page(page); + unlock_page(page); + return true; + } + unlock_page(page); + } + /* Drop
[PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
Memory fragmentation introduced by ballooning might reduce significantly the number of 2MB contiguous memory blocks that can be used within a guest, thus imposing performance penalties associated with the reduced number of transparent huge pages that could be used by the guest workload. This patch introduces the helper functions as well as the necessary changes to teach compaction and migration bits how to cope with pages which are part of a guest memory balloon, in order to make them movable by memory compaction procedures. Signed-off-by: Rafael Aquini aqu...@redhat.com --- include/linux/mm.h | 17 mm/compaction.c| 125 + mm/migrate.c | 30 - 3 files changed, 152 insertions(+), 20 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index 311be90..56cc553 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1662,5 +1662,22 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; } static inline bool page_is_guard(struct page *page) { return false; } #endif /* CONFIG_DEBUG_PAGEALLOC */ +#if (defined(CONFIG_VIRTIO_BALLOON) || \ + defined(CONFIG_VIRTIO_BALLOON_MODULE)) defined(CONFIG_COMPACTION) +extern bool isolate_balloon_page(struct page *); +extern void putback_balloon_page(struct page *); +extern struct address_space *balloon_mapping; + +static inline bool movable_balloon_page(struct page *page) +{ + return (page-mapping page-mapping == balloon_mapping); +} + +#else +static inline bool isolate_balloon_page(struct page *page) { return false; } +static inline void putback_balloon_page(struct page *page) { return false; } +static inline bool movable_balloon_page(struct page *page) { return false; } +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) CONFIG_COMPACTION */ + #endif /* __KERNEL__ */ #endif /* _LINUX_MM_H */ diff --git a/mm/compaction.c b/mm/compaction.c index e78cb96..e4e871b 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -14,6 +14,7 @@ #include linux/backing-dev.h #include linux/sysctl.h #include linux/sysfs.h +#include linux/export.h #include internal.h #if defined CONFIG_COMPACTION || defined CONFIG_CMA @@ -21,6 +22,84 @@ #define CREATE_TRACE_POINTS #include trace/events/compaction.h +#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE) +/* + * Balloon pages special page-mapping. + * Users must properly allocate and initialize an instance of balloon_mapping, + * and set it as the page-mapping for balloon enlisted page instances. + * There is no need on utilizing struct address_space locking schemes for + * balloon_mapping as, once it gets initialized at balloon driver, it will + * remain just like a static reference that helps us on identifying a guest + * ballooned page by its mapping, as well as it will keep the 'a_ops' callback + * pointers to the functions that will execute the balloon page mobility tasks. + * + * address_space_operations necessary methods for ballooned pages: + * .migratepage- used to perform balloon's page migration (as is) + * .invalidatepage - used to isolate a page from balloon's page list + * .freepage - used to reinsert an isolated page to balloon's page list + */ +struct address_space *balloon_mapping; +EXPORT_SYMBOL_GPL(balloon_mapping); + +static inline void __isolate_balloon_page(struct page *page) +{ + page-mapping-a_ops-invalidatepage(page, 0); +} + +static inline void __putback_balloon_page(struct page *page) +{ + page-mapping-a_ops-freepage(page); +} + +/* __isolate_lru_page() counterpart for a ballooned page */ +bool isolate_balloon_page(struct page *page) +{ + if (WARN_ON(!movable_balloon_page(page))) + return false; + + if (likely(get_page_unless_zero(page))) { + /* +* As balloon pages are not isolated from LRU lists, concurrent +* compaction threads can race against page migration functions +* move_to_new_page() __unmap_and_move(). +* In order to avoid having an already isolated balloon page +* being (wrongly) re-isolated while it is under migration, +* lets be sure we have the page lock before proceeding with +* the balloon page isolation steps. +*/ + if (likely(trylock_page(page))) { + /* +* A ballooned page, by default, has just one refcount. +* Prevent concurrent compaction threads from isolating +* an already isolated balloon page. +*/ + if (movable_balloon_page(page) + (page_count(page) == 2)) { + __isolate_balloon_page(page); + unlock_page(page); + return true; + } +