Re: [PATCH] thp+memcg-numa: fix BUG at include/linux/mm.h:370!

[Linus Torvalds]

On Mon, Mar 14, 2011 at 9:37 AM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
>
> I did try it that way at first (didn't help when I mistakenly put
> #ifndef instead of #ifdef around the put_page!), but was repulsed
> by seeing yet another #ifdef CONFIG_NUMA, so went with the duplicating
> version - which Linus has now taken.

I have to admit to being repulsed by the whole patch, but my main
source of "that's effin ugly" was from the crazy lock handling.

Does mem_cgroup_newpage_charge() even _need_ the mmap_sem at all? And
if not, why not release the read-lock early? And even if it _does_
need it, why not do

    ret = mem_cgroup_newpage_charge();
    up_read(&mm->mmap_sem);
    if (ret) {
        ...

finally, the #ifdef CONFIG_NUMA is ugly, but it's ugly in the return
path of the function too, and the nicer way would probably be to have
it in one place and do something like

    /*
     * The allocation rules are different for the NUMA/non-NUMA cases
     * For the NUMA case, we allocate here, for the non-numa case we
     * use the allocation in *hpage
     */
    static inline struct page *collapse_alloc_hugepage(struct page **hpage)
    {
    #ifdef CONFIG_NUMA
        VM_BUG_ON(*hpage);
        return alloc_hugepage_vma(khugepaged_defrag(), vma, address, node);
    #else
        VM_BUG_ON(!*hpage);
        return *hpage;
    #endif
    }

    static inline void collapse_free_hugepage(struct page *page)
    {
    #ifdef CONFIG_NUMA
        put_page(new_page);
    #else
        /* Nothing to do */
    #endif
    }

and use that instead. The point being that the #ifdef'fery now ends up
being in a much more targeted area and much better abstracted, rather
than in the middle of code, and ugly as sin.

But as mentioned, the lock handling is disgusting. Why is it even safe
to drop and re-take the lock at all?

                                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/