[Top] [All Lists]

Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()

To: Andi Kleen <andi@xxxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()
From: Felix Blyakher <felixb@xxxxxxx>
Date: Sat, 30 May 2009 09:57:20 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Hedi Berriche <hedi@xxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <87d49qeuqd.fsf@xxxxxxxxxxxxxxxxx>
References: <1243620631-10749-1-git-send-email-felixb@xxxxxxx> <1243620631-10749-2-git-send-email-felixb@xxxxxxx> <20090529192529.GA1599@xxxxxxxxxxxxx> <87d49qeuqd.fsf@xxxxxxxxxxxxxxxxx>

On May 30, 2009, at 5:43 AM, Andi Kleen wrote:

Christoph Hellwig <hch@xxxxxxxxxxxxx> writes:

diff --git a/fs/xfs/linux-2.6/kmem.h b/fs/xfs/linux-2.6/kmem.h
index af6843c..d8d2321 100644
--- a/fs/xfs/linux-2.6/kmem.h
+++ b/fs/xfs/linux-2.6/kmem.h
@@ -103,7 +103,7 @@ extern void *kmem_zone_zalloc(kmem_zone_t *, unsigned int __nocast);
static inline int
kmem_shake_allow(gfp_t gfp_mask)
-       return (gfp_mask & __GFP_WAIT) != 0;
+       return ((gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0;

Looks good to me.  But this could be written simpler as:

        return ((gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0;

That's actually not equivalent. You would need to write

(gfp_mask & (__GFP_WAIT|__GFP_FS)) == (__GFP_WAIT|__GFP_FS)


(gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0 is equivalent to

(gfp_mask & __GFP_WAIT || gfp_mask & __GFP_FS) != 0;

or "if _any_ of the flags set".
In the fix we need "_both_  flags set", which brings us to
equally (IMHO) readable

(gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS)  != 0   or as Andi noted
(gfp_mask & (__GFP_WAIT|__GFP_FS)) == (__GFP_WAIT|__GFP_FS)

I'd prefer the former, as in my original patch.

Also, I accidentally put an extra open brace in a statement. After a
successful build I started playing with braces for more readability,
and left it in inconsistent state.
Seems like the preferred style in the kernel is as following:

return ((gfp_mask & __GFP_WAIT) && (gfp_mask & __GFP_FS)) != 0;

If there is no objection, I'll use that.


which is not simpler.

BTW you can usually trust gcc to do all the obvious logical
simplifications, so it's best to just write the most readable code
(like Felix's original version) And also it can depend on the target
what is actually better in machine language, and gcc has more
knowledge about that than the programmer.

int f(int x)
       return x & 1 && x & 2;

gives for gcc 4.3

andl    $3, %edi
xorl    %eax, %eax
cmpl    $3, %edi
sete    %al


ak@xxxxxxxxxxxxxxx -- Speaking for myself only.

<Prev in Thread] Current Thread [Next in Thread>