Eric Dumazet [off-list ref] writes:

On Wed, 2015-11-25 at 14:32 -0800, Eric Dumazet wrote:

quoted

On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote:

quoted

quoted

diff --git a/include/net/sock.h b/include/net/sock.h
index 7f89e4ba18d1..ead514332ae8 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

@@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
 
 static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
 {
-	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
+	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
 }
 

Sorry if I'm missing something obvious here, but with a name like
sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well?
Otherwise, what is the _atomic is saying?

Not sure what you suggest.

Are you suggesting I remove GFP_ATOMIC from all callers ?

That's an option, and one that looks pretty clean.

quoted

I am fine with this, but looks more invasive, and who knows, maybe one
caller might want to not use GFP_ATOMIC one day (like : do not attempt
to use reserves)

Probably that would call for a different more primitive version of this
API (sk_gfp_or_memalloc() as you suggest below). Then this could be
written in terms of that

static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask)
{
	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
}

static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
{
	return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC);
}

Not sure if it's "too much API".

quoted

This sk_gfp_atomic() helper has a misleading name, since all it wanted
was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one)
with __GFP_MEMALLOC for some special sockets.

Should have been sk_gfp_or_memalloc() or something...

BTW original commit changelog was clear and matches my expectations :

commit 99a1dec70d5acbd8c6b3928cdebb4a2d1da676c8
Author: Mel Gorman [off-list ref]
Date:   Tue Jul 31 16:44:14 2012 -0700

    net: introduce sk_gfp_atomic() to allow addition of GFP flags
depending on the individual socket
    
    Introduce sk_gfp_atomic(), this function allows to inject sock specific
    flags to each sock related allocation.  It is only used on allocation
    paths that may be required for writing pages back to network storage.

Cool. If you think my suggestion is too much for this set, that's
fine. I understand not wanting to be too intrusive.

Thanks,
Aaron