lkml.org 
[lkml]   [2004]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [patch 1/3] memory hotplug prototype
From
Date
On Tue, 2004-04-06 at 03:56, IWAMOTO Toshihiro wrote:
> for (node = local_node + 1; node < numnodes; node++)
> - j = build_zonelists_node(NODE_DATA(node), zonelist, j, k);
> + j = build_zonelists_node(NODE_DATA(node),
> + zonelist, j, k);

This line is probably too long already, but please leave it. If you
want to clean things up, please send a separate cleanup patch.

> -/* Zone modifiers in GFP_ZONEMASK (see linux/mmzone.h - low two bits) */
> -#define __GFP_DMA 0x01
> -#define __GFP_HIGHMEM 0x02
> +/* Zone modifiers in GFP_ZONEMASK (see linux/mmzone.h - low three bits) */
> +#define __GFP_DMA 0x01
> +#define __GFP_HIGHMEM 0x02
> +#define __GFP_HOTREMOVABLE 0x03

Are you still determined to add a zone like this? Hotplug will
eventually have to be done on all of the zones (NORMAL, DMA, etc...), so
it seems a bit shortsighted to add a zone like this. I think it would
be much more valuable to be able to attempt hotremove operations on any
zone, even if they are unlikely to succeed.

> #define put_page_testzero(p) \
> ({ \
> - BUG_ON(page_count(p) == 0); \
> + if (page_count(p) == 0) { \
> + int i; \
> + printk("Page: %lx ", (long)p); \
> + for(i = 0; i < sizeof(struct page); i++) \
> + printk(" %02x", ((unsigned char *)p)[i]); \
> + printk("\n"); \
> + BUG(); \
> + } \
> atomic_dec_and_test(&(p)->count); \
> })

Could you pull this debugging code out, or put it in an out-of-line
function? Stuff like this in inline functions or macros just bloat
code.

> -#define try_to_unmap(page) SWAP_FAIL
> +#define try_to_unmap(page, force) SWAP_FAIL
> #endif /* CONFIG_MMU *
...
> +#ifdef CONFIG_MEMHOTPLUG
> + vlist = kmalloc(sizeof(struct page_va_list), GFP_KERNEL);
> + vlist->mm = mm;
> + vlist->addr = address;
> + list_add(&vlist->list, force);
> +#endif


Could you explain what you're trying to do with try_to_unmap() and why
all of the calls need to be changed?

> - int error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + int error = radix_tree_preload((gfp_mask & ~GFP_ZONEMASK) |
> + ((gfp_mask & GFP_ZONEMASK) == __GFP_DMA ? __GFP_DMA : 0));

What is this doing? Trying to filter off the highmem flag without
affecting the hotremove flag???

> lock_page(page);
> + if (page->mapping == NULL) {
> + BUG_ON(! PageAgain(page));
> + unlock_page(page);
> + page_cache_release(page);
> + pte_chain_free(pte_chain);
> + goto again;
> + }
> + BUG_ON(PageAgain(page));

You might want to add a little comment here noting that this is for the
hotremove case only. No normal paths hit it.


> -#if defined(CONFIG_PM) || defined(CONFIG_HOTPLUG_CPU)
> +#if defined(CONFIG_PM) || defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMHOTPLUG)

I think it's safe to say that you need an aggregate config option for
this one :)

> /*
> * Builds allocation fallback zone lists.
> */
> -static int __init build_zonelists_node(pg_data_t *pgdat, struct zonelist *zonelist, int j, int k)
> +static int build_zonelists_node(pg_data_t *pgdat, struct zonelist *zonelist, int j, int k)
> {
> +
> + if (! pgdat->enabled)
> + return j;

Normal Linux style is:
> + if (!pgdat->enabled)
> + return j;


> + if (k != ZONE_HOTREMOVABLE &&
> + pgdat->removable)
> + return j;

What is this check supposed to do?


> - memset(zonelist, 0, sizeof(*zonelist));
> + /* memset(zonelist, 0, sizeof(*zonelist)); */

Why is this memset unnecessary now?


> j = 0;
> k = ZONE_NORMAL;
> - if (i & __GFP_HIGHMEM)
> + hotremovable = 0;
> + switch (i) {
> + default:
> + BUG();
> + return;
> + case 0:
> + k = ZONE_NORMAL;
> + break;
> + case __GFP_HIGHMEM:
> k = ZONE_HIGHMEM;
> - if (i & __GFP_DMA)
> + break;
> + case __GFP_DMA:
> k = ZONE_DMA;
> + break;
> + case __GFP_HOTREMOVABLE:
> +#ifdef CONFIG_MEMHOTPLUG
> + k = ZONE_HIGHMEM;
> +#else
> + k = ZONE_HOTREMOVABLE;
> +#endif
> + hotremovable = 1;
> + break;
> + }

What if, in the header you did this:
#ifndef CONFIG_MEMHOTPLUG
#define ZONE_HOTREMOVABLE ZONE_HIGHMEM
#endif

Then, you wouldn't need the #ifdef in the .c file.

There is way too much ifdef'ing going on in this code. The general
Linux rule is: no #ifdef's in .c files. This is a good example why :)

> +#ifndef CONFIG_MEMHOTPLUG
> j = build_zonelists_node(pgdat, zonelist, j, k);
> /*
> * Now we build the zonelist so that it contains the zones
> @@ -1267,22 +1304,59 @@ static void __init build_zonelists(pg_da
> * node N+1 (modulo N)
> */
> for (node = local_node + 1; node < numnodes; node++)
> - j = build_zonelists_node(NODE_DATA(node), zonelist, j, k);
> + j = build_zonelists_node(NODE_DATA(node),
> + zonelist, j, k);
> for (node = 0; node < local_node; node++)
> - j = build_zonelists_node(NODE_DATA(node), zonelist, j, k);
> -
> - zonelist->zones[j++] = NULL;
> + j = build_zonelists_node(NODE_DATA(node),
> + zonelist, j, k);
> +#else
> + while (hotremovable >= 0) {
> + for(; k >= 0; k--) {
> + zone = pgdat->node_zones + k;
> + for (node = local_node; ;) {
> + if (NODE_DATA(node) == NULL ||
> + ! NODE_DATA(node)->enabled ||
> + (!! NODE_DATA(node)->removable) !=
> + (!! hotremovable))
> + goto next;
> + zone = NODE_DATA(node)->node_zones + k;
> + if (zone->present_pages)
> + zonelist->zones[j++] = zone;
> + next:
> + node = (node + 1) % numnodes;
> + if (node == local_node)
> + break;
> + }
> + }
> + if (hotremovable) {
> + /* place non-hotremovable after hotremovable */
> + k = ZONE_HIGHMEM;
> + }
> + hotremovable--;
> + }
> +#endif
> + BUG_ON(j > sizeof(zonelist->zones) /
> + sizeof(zonelist->zones[0]) - 1);
> + for(; j < sizeof(zonelist->zones) /
> + sizeof(zonelist->zones[0]); j++)
> + zonelist->zones[j] = NULL;
> }
> }

That code need to be separated out somehow. It's exceedingly hard to
understand what's going on.

> -void __init build_all_zonelists(void)
> +#ifdef CONFIG_MEMHOTPLUG
> +void
> +#else
> +void __init
> +#endif
> +build_all_zonelists(void)
> {
> int i;
>
> for(i = 0 ; i < numnodes ; i++)
> - build_zonelists(NODE_DATA(i));
> + if (NODE_DATA(i) != NULL)
> + build_zonelists(NODE_DATA(i));
> printk("Built %i zonelists\n", numnodes);
> }

Please make that __init __devinit, instead of using the #ifdef. That
will turn off the __init when CONFIG_HOTPLUG is turned on. BTW, you
should make HOTPLUGMEM dependent on CONFIG_HOTPLUG.


-- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:02    [from the cache]
©2003-2011 Jasper Spaans