Re: [patch 1/3] memory hotplug prototype

[IWAMOTO Toshihiro]

At Tue, 06 Apr 2004 10:12:58 -0700,
Dave Hansen wrote:
> > -/* 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

Hotremoval things are a bit confusing.
There are __GFP_HOTREMOVABLE macro for alloc_page and
node_zonelists[ZONE_HOTREMOVABLE], but there's no
node_zones[ZONE_HOTREMOVABLE].  Hotremovable attribute of a zone is
stored in its pgdat.
And every zone can be hotremovable.

> > -#define try_to_unmap(page)	SWAP_FAIL
> > +#define try_to_unmap(page, force)	SWAP_FAIL
> >  #endif /* CONFIG_MMU *

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

This is necessary for handling mlocked pages.

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

Because __GFP_HOTREMOVABLE & ~ __GFP_HIGHMEM evaluates to __GFP_DMA.


> >  /*
> >   * 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;

Ok.

> > +	if (k != ZONE_HOTREMOVABLE &&
> > +	    pgdat->removable)
> > +		return j;
> 
> What is this check supposed to do?

This code was used when build_zonelists_node was called from
build_zonelists even if CONFIG_MEMHOTPLUG was defined.
So, this diff is no longer necessary.

> > -		memset(zonelist, 0, sizeof(*zonelist));
> > +		/* memset(zonelist, 0, sizeof(*zonelist)); */
> 
> Why is this memset unnecessary now?

This diff is also no longer necessary, but unused tail elements of
zonelist is zeroed in build_zonelists anyway.

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