On 06/09/2014 11:09 AM, David Rientjes wrote:
On Fri, 6 Jun 2014, Vlastimil Babka wrote:
diff --git a/mm/internal.h b/mm/internal.h
index 1a8a0d4..6aa1f74 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -164,7 +164,8 @@ isolate_migratepages_range(struct zone *zone, struct
compact_control *cc,
* general, page_zone(page)->lock must be held by the caller to prevent
the
* page from being allocated in parallel and returning garbage as the
order.
* If a caller does not hold page_zone(page)->lock, it must guarantee
that the
- * page cannot be allocated or merged in parallel.
+ * page cannot be allocated or merged in parallel. Alternatively, it must
+ * handle invalid values gracefully, and use page_order_unsafe() below.
*/
static inline unsigned long page_order(struct page *page)
{
@@ -172,6 +173,23 @@ static inline unsigned long page_order(struct page
*page)
return page_private(page);
}
+/*
+ * Like page_order(), but for callers who cannot afford to hold the zone
lock,
+ * and handle invalid values gracefully. ACCESS_ONCE is used so that if
the
+ * caller assigns the result into a local variable and e.g. tests it for
valid
+ * range before using, the compiler cannot decide to remove the variable
and
+ * inline the function multiple times, potentially observing different
values
+ * in the tests and the actual use of the result.
+ */
+static inline unsigned long page_order_unsafe(struct page *page)
+{
+ /*
+ * PageBuddy() should be checked by the caller to minimize race
window,
+ * and invalid values must be handled gracefully.
+ */
+ return ACCESS_ONCE(page_private(page));
+}
+
/* mm/util.c */
void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
struct vm_area_struct *prev, struct rb_node *rb_parent);
I don't like this change at all, I don't think we should have header
functions that imply the context in which the function will be called. I
think it would make much more sense to just do
ACCESS_ONCE(page_order(page)) in the migration scanner with a comment.
But that won't compile. It would have to be converted to a #define, unless
there's some trick I don't know. Sure I would hope this could be done cleaner
somehow.
Sorry, I meant ACCESS_ONCE(page_private(page)) in the migration scanner
Hm but that's breaking the abstraction of page_order(). I don't know if it's
worse to create a new variant of page_order() or to do this. BTW, seems like
next_active_pageblock() in memory-hotplug.c should use this variant too.
The compiler seems free to disregard the access of a volatile object above
because the return value of the inline function is unsigned long. What's
the difference between unsigned long order = page_order_unsafe(page) and
unsigned long order = (unsigned long)ACCESS_ONCE(page_private(page)) and
I think there's none functionally, but one is abstraction layer
violation and the other imply the context of usage as you say (but is
that so uncommon?).
the compiler being able to reaccess page_private() because the result is
no longer volatile qualified?
You think it will reaccess? That would defeat all current ACCESS_ONCE
usages, no?
What I'm trying to prevent is that this code:
unsigned long freepage_order = page_order(page);
if (freepage_order > 0 && freepage_order < MAX_ORDER)
low_pfn += (1UL << freepage_order) - 1;
could be effectively changed (AFAIK legal for the compiler to do) to:
if (page_order(page) > 0 && page_order(page) < MAX_ORDER)
low_pfn += (1UL << page_order(page)) - 1;
And thus check a different value than it's in the end used to bump low_pfn.
I believe that even though freepage_order itself is not volatile, the
fact it was assigned through a volatile cast means the compiler won't be
able to do this anymore.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>