Re: [Patch 002/002] Create/delete kmem_cache_node for SLUB on memoryonline callback

[Christoph Lameter]

On Fri, 12 Oct 2007, Yasunori Goto wrote:
 
> If pages on the new node available, slub can use it before making
> new kmem_cache_nodes. So, this callback should be called
> BEFORE pages on the node are available.

If its called before pages on the node are available then it must 
fallback and cannot use the pages.

> +#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
> +static int slab_mem_going_offline_callback(void *arg)
> +{
> +	struct kmem_cache *s;
> +	struct memory_notify *marg = arg;
> +	int local_node, offline_node = marg->status_change_nid;
> +
> +	if (offline_node < 0)
> +		/* node has memory yet. nothing to do. */

Please clarify the comment. This seems to indicate that we should not
do anything because the node still has memory?

Doesnt the node always have memory before offlining?

> +		return 0;
> +
> +	down_read(&slub_lock);
> +	list_for_each_entry(s, &slab_caches, list) {
> +		local_node = page_to_nid(virt_to_page(s));
> +		if (local_node == offline_node)
> +			/* This slub is on the offline node. */
> +			return -EBUSY;
> +	}
> +	up_read(&slub_lock);

So this checks if the any kmem_cache structure is on the offlined node? If
so then we cannot offline the node?


> +	kmem_cache_shrink_node(s, offline_node);

kmem_cache_shrink(s) would be okay here I would think. The function is
reasonably fast. Offlining is a rare event.

> +static void slab_mem_offline_callback(void *arg)

We call this after we have established that no kmem_cache structures are 
on this and after we have shrunk the slabs. Is there any guarantee that
no slab operations have occurrent since then?

> +{
> +	struct kmem_cache_node *n;
> +	struct kmem_cache *s;
> +	struct memory_notify *marg = arg;
> +	int offline_node;
> +
> +	offline_node = marg->status_change_nid;
> +
> +	if (offline_node < 0)
> +		/* node has memory yet. nothing to do. */
> +		return;

Does this mean that the node still has memory?

> +	down_read(&slub_lock);
> +	list_for_each_entry(s, &slab_caches, list) {
> +		n = get_node(s, offline_node);
> +		if (n) {
> +			/*
> +			 * if n->nr_slabs > 0, offline_pages() must be fail,
> +			 * because the node is used by slub yet.
> +			 */

It may be clearer to say:

"If nr_slabs > 0 then slabs still exist on the node that is going down.
We were unable to free them so we must fail."

> +static int slab_mem_going_online_callback(void *arg)
> +{
> +	struct kmem_cache_node *n;
> +	struct kmem_cache *s;
> +	struct memory_notify *marg = arg;
> +	int nid = marg->status_change_nid;
> +
> +	/* If the node already has memory, then nothing is necessary. */
> +	if (nid < 0)
> +		return 0;

The node must have memory???? Or we have already brought up the code?

> +	/*
> +	 * New memory will be onlined on the node which has no memory so far.
> +	 * New kmem_cache_node is necssary for it.

"We are bringing a node online. No memory is available yet. We must 
allocate a kmem_cache_node structure in order to bring the node online." ?

> +	 */
> +	down_read(&slub_lock);
> +	list_for_each_entry(s, &slab_caches, list) {
> +  		/*
> +		 * XXX: The new node's memory can't be allocated yet,
> +		 *      kmem_cache_node will be allocated other node.
> +  		 */

"kmem_cache_alloc node will fallback to other nodes since memory is 
not yet available from the node that is brought up.¨ ?

> +		n = kmem_cache_alloc(kmalloc_caches, GFP_KERNEL);
> +		if (!n)
> +			return -ENOMEM;
> +		init_kmem_cache_node(n);
> +		s->node[nid] = n;
> +  	}
> +	up_read(&slub_lock);
> +
> +  	return 0;
> +}