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:
quoted
quoted
+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?
quoted
quoted
+#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.
quoted
quoted
+/* __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.
quoted
+ 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.
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:
quoted
On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote:
quoted
quoted
+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?
quoted
quoted
+#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.
quoted
quoted
quoted
+/* __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.
quoted
+ 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?
quoted
While testing it, I wasn't lucky to see this small window opening, though.
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote:
quoted
quoted
quoted
+/* __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.
quoted
+ 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?
quoted
While testing it, I wasn't lucky to see this small window opening, though.