Messages in this thread | | | Date | Thu, 11 Oct 2007 21:09:06 -0700 (PDT) | From | Christoph Lameter <> | Subject | Re: [Patch 002/002] Create/delete kmem_cache_node for SLUB on memory online callback |
| |
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; > +} | |