netdev - Re: [PATCH 10/17] netvm: Allow skb allocation to use PFMEMALLOC reserves

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 linux-hardening linux-cve-announce 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]

Message-ID: <20120511143218.GS11435@suse.de>
Date:	Fri, 11 May 2012 15:32:18 +0100
From:	Mel Gorman <mgorman@...e.de>
To:	David Miller <davem@...emloft.net>
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 10/17] netvm: Allow skb allocation to use PFMEMALLOC
 reserves

On Fri, May 11, 2012 at 12:57:40AM -0400, David Miller wrote:
> From: Mel Gorman <mgorman@...e.de>
> Date: Thu, 10 May 2012 14:45:03 +0100
> 
> > +/* Returns true if the gfp_mask allows use of ALLOC_NO_WATERMARK */
> > +bool gfp_pfmemalloc_allowed(gfp_t gfp_mask);
> 
> I know this gets added in an earlier patch, but it seems slightly
> overkill to have a function call just for a simply bit test.
> 

It's not that simple. gfp_pfmemalloc_allowed calls gfp_to_alloc_flags()
which is quite involved and probably should not be duplicated. In the slab
case, it's called from slow paths where we are already under memory pressure
and swapping to network so it's not a major problem. In the network case,
it is called when kmalloc() has already failed and also a slow path.

> > +extern atomic_t memalloc_socks;
> > +static inline int sk_memalloc_socks(void)
> > +{
> > +	return atomic_read(&memalloc_socks);
> > +}
> 
> Please change this to be a static branch.
> 

Will do. I renamed memalloc_socks to sk_memalloc_socks, made it a int as
atomics are unnecessary and I check it directly in a branch instead of a
static inline. It should be relatively easy for the branch predictor.

> > +	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask,
> > +						SKB_ALLOC_RX, NUMA_NO_NODE);
> 
> Please fix the argument indentation.
> 

Done.

> > +	data = kmalloc_reserve(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > +		       gfp_mask, NUMA_NO_NODE, NULL);
> 
> Likewise.

Done

> 
> > -	struct sk_buff *n = alloc_skb(newheadroom + skb->len + newtailroom,
> > -				      gfp_mask);
> > +	struct sk_buff *n = __alloc_skb(newheadroom + skb->len + newtailroom,
> > +				      gfp_mask, skb_alloc_rx_flag(skb),
> > +				      NUMA_NO_NODE);
> 
> Likewise.
> 

Done.

> > -			nskb = alloc_skb(hsize + doffset + headroom,
> > -					 GFP_ATOMIC);
> > +			nskb = __alloc_skb(hsize + doffset + headroom,
> > +					 GFP_ATOMIC, skb_alloc_rx_flag(skb),
> > +					 NUMA_NO_NODE);
> 
> Likewise.

Done.

Thanks.

-- 
Mel Gorman
SUSE Labs
--
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