netdev - Re: [PATCH 12/30] mm: memory reserve management

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: <1218528658.10800.173.camel@twins>
Date:	Tue, 12 Aug 2008 10:10:58 +0200
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	Neil Brown <neilb@...e.de>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	netdev@...r.kernel.org, trond.myklebust@....uio.no,
	Daniel Lezcano <dlezcano@...ibm.com>,
	Pekka Enberg <penberg@...helsinki.fi>
Subject: Re: [PATCH 12/30] mm: memory reserve management

On Tue, 2008-08-12 at 16:23 +1000, Neil Brown wrote:
> On Thursday July 24, a.p.zijlstra@...llo.nl wrote:
> > Generic reserve management code. 
> > 
> > It provides methods to reserve and charge. Upon this, generic alloc/free style
> > reserve pools could be build, which could fully replace mempool_t
> > functionality.
> 
> This looks quite different to last time I looked at the code (I
> think).
> 
> You now have a more structured "kmalloc_reserve" interface which
> returns a flag to say if the allocation was from an emergency pool.  I
> think this will be a distinct improvement at the call sites, though I
> haven't looked at them yet. :-)
> 
> > +
> > +struct mem_reserve {
> > +	struct mem_reserve *parent;
> > +	struct list_head children;
> > +	struct list_head siblings;
> > +
> > +	const char *name;
> > +
> > +	long pages;
> > +	long limit;
> > +	long usage;
> > +	spinlock_t lock;	/* protects limit and usage */
>                                             ^^^^^
> > +
> > +	wait_queue_head_t waitqueue;
> > +};
> 
> .....
> > +static void __calc_reserve(struct mem_reserve *res, long pages, long limit)
> > +{
> > +	unsigned long flags;
> > +
> > +	for ( ; res; res = res->parent) {
> > +		res->pages += pages;
> > +
> > +		if (limit) {
> > +			spin_lock_irqsave(&res->lock, flags);
> > +			res->limit += limit;
> > +			spin_unlock_irqrestore(&res->lock, flags);
> > +		}
> > +	}
> > +}
> 
> I cannot figure out why the spinlock is being used to protect updates
> to 'limit'.
> As far as I can see, mem_reserve_mutex already protects all those
> updates.
> Certainly we need the spinlock for usage, but why for limit??

against __mem_reserve_charge(), granted, the race would be minimal at
best - but it seemed better this way.

> > +
> > +void *___kmalloc_reserve(size_t size, gfp_t flags, int node, void *ip,
> > +			 struct mem_reserve *res, int *emerg)
> > +{
> .....
> > +	if (emerg)
> > +		*emerg |= 1;
> 
> Why not just
> 
> 	if (emerg)
> 		*emerg = 1.
> 
> I can't we where '*emerg' can have any value but 0 or 1, so the '|' is
> pointless ???

weirdness in my brain when I wrote that I guess, shall ammend!

--
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