lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 11 May 2012 00:49:49 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	mgorman@...e.de
Cc:	akpm@...ux-foundation.org, linux-mm@...ck.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	neilb@...e.de, a.p.zijlstra@...llo.nl, michaelc@...wisc.edu,
	emunson@...bm.net
Subject: Re: [PATCH 08/17] net: Introduce sk_allocation() to allow addition
 of GFP flags depending on the individual socket

From: Mel Gorman <mgorman@...e.de>
Date: Thu, 10 May 2012 14:45:01 +0100

> Introduce sk_allocation(), 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.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Signed-off-by: Mel Gorman <mgorman@...e.de>

This is still a little bit more than it needs to be.

You are trying to propagate a single bit from sk->sk_allocation into
all of the annotated socket memory allocation sites.

But many of them use sk->sk_allocation already.  In fact all of them
that use a variable rather than a constant GFP_* satisfy this
invariant.

All of those annotations are therefore spurious, and probably end up
generating unnecessary |'s in of that special bit in at least some
cases.

What you really, therefore, care about are the GFP_FOO cases.  And in
fact those are all GFP_ATOMIC.  So make something that says what it
is that you want, a GFP_ATOMIC with some socket specified bits |'d
in.

Something like this:

static inline gfp_t sk_gfp_atomic(struct sock *sk)
{
	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
}

You'll also have to make your networking patches conform to the
networking subsystem coding style.

For example:

> -	skb = sock_wmalloc(sk, MAX_TCP_HEADER + 15 + s_data_desired, 1, GFP_ATOMIC);
> +	skb = sock_wmalloc(sk, MAX_TCP_HEADER + 15 + s_data_desired, 1,
> +					sk_allocation(sk, GFP_ATOMIC));

The sk_allocation() argument has to line up with the first column
after the openning parenthesis of the function call.  You can't just
use all TAB characters.  And this all TABs thing looks extremely ugly
to boot.

> -		newnp->pktoptions = skb_clone(treq->pktopts, GFP_ATOMIC);
> +		newnp->pktoptions = skb_clone(treq->pktopts,
> +						sk_allocation(sk, GFP_ATOMIC));

Same here.

What's really funny to me is that in several cases elsewhere in this
pach you get it right.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists