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

Neil Brown
2008-08-12T06:23:36Z

On Thursday July 24, a.p.zijlstra@chello.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??

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

Thanks,
NeilBrown