On Wed, Jun 10, 2015 at 05:44:30PM +0200, Vlastimil Babka wrote:
> On 06/03/2015 07:06 PM, Kirill A. Shutemov wrote:
> >+static int __split_huge_page_tail(struct page *head, int tail,
> >+ struct lruvec *lruvec, struct list_head *list)
> >+{
> >+ int mapcount;
> >+ struct page *page_tail = head + tail;
> >+
> >+ mapcount = page_mapcount(page_tail);
>
> Isn't page_mapcount() unnecessarily heavyweight here? When you are splitting
> a page, it already should have zero compound_mapcount() and shouldn't be
> PageDoubleMap(), no? So you should care about page->_mapcount only? Sure,
> splitting THP is not a hotpath, but when done 512 times per split, it could
> make some difference in the split's latency.
Okay, replaced with direct atomic_read().
> >+ VM_BUG_ON_PAGE(atomic_read(&page_tail->_count) != 0, page_tail);
> >+
> >+ /*
> >+ * tail_page->_count is zero and not changing from under us. But
> >+ * get_page_unless_zero() may be running from under us on the
> >+ * tail_page. If we used atomic_set() below instead of atomic_add(), we
> >+ * would then run atomic_set() concurrently with
> >+ * get_page_unless_zero(), and atomic_set() is implemented in C not
> >+ * using locked ops. spin_unlock on x86 sometime uses locked ops
> >+ * because of PPro errata 66, 92, so unless somebody can guarantee
> >+ * atomic_set() here would be safe on all archs (and not only on x86),
> >+ * it's safer to use atomic_add().
>
> I would be surprised if this was the first place to use atomic_set() with
> potential concurrent atomic_add(). Shouldn't atomic_*() API guarantee that
> this works?
I don't have much insight on the issue. This part is carried over from
pre-rework split_huge_page().
>
> >+ */
> >+ atomic_add(page_mapcount(page_tail) + 1, &page_tail->_count);
>
> You already have the value in mapcount variable, so why read it again.
Fixed.
--
Kirill A. Shutemov
--
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>