On Tue, Sep 23, 2014 at 12:22 AM, Rafael Aquini <aquini@xxxxxxxxxx> wrote: > On Tue, Sep 23, 2014 at 12:06:11AM +0400, Konstantin Khlebnikov wrote: >> On Mon, Sep 22, 2014 at 11:22 PM, Rafael Aquini <aquini@xxxxxxxxxx> wrote: >> > On Mon, Sep 22, 2014 at 10:40:34PM +0400, Konstantin Khlebnikov wrote: >> >> On Sat, Sep 20, 2014 at 10:23 AM, Andrew Morton >> >> <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: >> >> > On Sat, 20 Sep 2014 09:25:01 +0400 Konstantin Khlebnikov <koct9i@xxxxxxxxx> wrote: >> >> > >> >> >> > >> >> >> > So I'm going to send "fix for >> >> >> > mm-balloon_compaction-use-common-page-ballooning-v2" to Linus >> >> >> > separately, but it has no changelog at all. >> >> >> >> >> >> Probably it would be better if you drop everything except actually >> >> >> fixes and stresstest. This is gone too far, now balloon won't compile >> >> >> in the middle of patchset. Just tell me and I'll redo the rest. >> >> > >> >> > I think it's best if I drop everything: >> >> > >> >> > mm-balloon_compaction-ignore-anonymous-pages.patch >> >> > mm-balloon_compaction-keep-ballooned-pages-away-from-normal-migration-path.patch >> >> > mm-balloon_compaction-isolate-balloon-pages-without-lru_lock.patch >> >> > selftests-vm-transhuge-stress-stress-test-for-memory-compaction.patch >> >> > mm-introduce-common-page-state-for-ballooned-memory.patch >> >> > mm-balloon_compaction-use-common-page-ballooning.patch >> >> > mm-balloon_compaction-general-cleanup.patch >> >> > mm-balloon_compaction-use-common-page-ballooning-v2-fix-1.patch >> >> > >> >> > Please go through it and send out a new version? >> >> > >> >> > >> >> >> >> I've found yet another bug in this code. It seems here is a nest. >> >> balloon_page_dequeue can race with balloon_page_isolate: >> >> balloon_page_isolate can remove page from list between >> >> llist_for_each_entry_safe and trylock_page in balloon_page_dequeue. >> >> balloon_page_dequeue runs under mutex_lock(&vb->balloon_lock); >> >> both of them lock page using trylock_page so race is tight but it is >> >> not impossible. >> > Plausible to happen if stress testing compaction simultaneously with >> > freezing/unloading the balloon driver. As you noted, it's quite tight >> > despite not impossible. Nice catch. >> > >> > >> >> Probably it's really easier to rewrite it than to fix bugs one by one =/ >> > I'm not against a rewrite, but I don't think that rewriting the code to get rid >> > of such bugs changes the fact we still have to address them in the actual placed >> > code as we go on finding them. That's why I thought your inital changeset fine, >> > with patches for stable going first and code overhaul for next following them up. >> > >> > For this race you spotted, I think a simple change like the following >> > might be enough (not-tested) >> >> This locking scheme is too fragile and uncommon. >> > > page_lock and refcounting was what I had at my disposal to sort these > races out since I haven't thoutgh on a special page->_mapcount when > designing this feature. It's the way other page races are sorted out. > Not arguing it's not fragile, but it's the way code is layed out since > some time, so we must check the feasibility of a total overhaul for > stable branches. > > >> What about this: >> >> * special page->_mapcount marks ballooned pages >> * page->private points to balloon (directly, without intermediate mapping) >> * flag PagePrivate means page currently in balloon page list (i.e. not >> isolated, like PageLRU for normal pages) >> * lock_page protects all of them >> >> balloon_page_dequeue() will delete page from balloon list only if it's >> not isolated, also it always clears page->private and balloon mark. >> put-back rechecks mark after locking the page and releases it as >> normal page if mark is gone. >> > > I have already agreed with you here, since the changes above are mostly from > your original overhaul proposal. It's a much better approach for that > balloon code, no doubts. Thanks for doing it. Only thing we need to take > care here is about its requirement on changing the semantics for those > interfaces might turn the changes unfeasible for old stable branches. If we > can ignore this mentioned fact entirely, I don't see why not going with > your idea all branches across, otherwise I think we should overhaul the > code for -next, and send pontual fixes for stable. > Ok, I'll try to implement this approach in suitable for stable branches way. Keeping several different versions is overkill for this code. > > Cheers, > -- Rafael > >> > >> > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >> > index 6e45a50..fd3a497 100644 >> > --- a/mm/balloon_compaction.c >> > +++ b/mm/balloon_compaction.c >> > @@ -93,6 +93,16 @@ struct page *balloon_page_dequeue(struct >> > balloon_dev_info *b_dev_info) >> > * to be released by the balloon driver. >> > */ >> > if (trylock_page(page)) { >> > + /* >> > + * Skip dequeue attempt for this page to a later round >> > + * if balloon_page_isolate() has sucessfully isolated >> > + * it just before we got the page lock here. >> > + */ >> > + if (page_count(page) != 1) { >> > + unlock_page(page); >> > + continue >> > + } >> > + >> > spin_lock_irqsave(&b_dev_info->pages_lock, flags); >> > /* >> > * Raise the page refcount here to prevent any >> > * wrong >> > -- 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>