Re: [PATCH 1/5] v2 Split the memory_block structure

[Nathan Fontenot <nfont@austin.ibm.com>]

On 07/16/2010 12:15 PM, Dave Hansen wrote:
> On Thu, 2010-07-15 at 13:37 -0500, Nathan Fontenot wrote:
>> @@ -123,13 +130,20 @@
>>  static ssize_t show_mem_removable(struct sys_device *dev,
>>  			struct sysdev_attribute *attr, char *buf)
>>  {
>> +	struct memory_block *mem;
>> +	struct memory_block_section *mbs;
>>  	unsigned long start_pfn;
>> -	int ret;
>> -	struct memory_block *mem =
>> -		container_of(dev, struct memory_block, sysdev);
>> +	int ret = 1;
>> +
>> +	mem = container_of(dev, struct memory_block, sysdev);
>> +	mutex_lock(&mem->state_mutex);
>>
>> -	start_pfn = section_nr_to_pfn(mem->phys_index);
>> -	ret = is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
>> +	list_for_each_entry(mbs, &mem->sections, next) {
>> +		start_pfn = section_nr_to_pfn(mbs->phys_index);
>> +		ret &= is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
>> +	}
>> +
>> +	mutex_unlock(&mem->state_mutex);
>>  	return sprintf(buf, "%d\n", ret);
>>  }
> 
> Now that the "state_mutex" is getting used for other stuff, should we
> just make it "mutex"?
> 
>> @@ -182,16 +196,16 @@
>>   * OK to have direct references to sparsemem variables in here.
>>   */
>>  static int
>> -memory_block_action(struct memory_block *mem, unsigned long action)
>> +memory_block_action(struct memory_block_section *mbs, unsigned long action)
>>  {
>>  	int i;
>>  	unsigned long psection;
>>  	unsigned long start_pfn, start_paddr;
>>  	struct page *first_page;
>>  	int ret;
>> -	int old_state = mem->state;
>> +	int old_state = mbs->state;
>>
>> -	psection = mem->phys_index;
>> +	psection = mbs->phys_index;
>>  	first_page = pfn_to_page(psection << PFN_SECTION_SHIFT);
>>
>>  	/*
>> @@ -217,18 +231,18 @@
>>  			ret = online_pages(start_pfn, PAGES_PER_SECTION);
>>  			break;
>>  		case MEM_OFFLINE:
>> -			mem->state = MEM_GOING_OFFLINE;
>> +			mbs->state = MEM_GOING_OFFLINE;
>>  			start_paddr = page_to_pfn(first_page) << PAGE_SHIFT;
>>  			ret = remove_memory(start_paddr,
>>  					    PAGES_PER_SECTION << PAGE_SHIFT);
>>  			if (ret) {
>> -				mem->state = old_state;
>> +				mbs->state = old_state;
>>  				break;
>>  			}
>>  			break;
>>  		default:
>>  			WARN(1, KERN_WARNING "%s(%p, %ld) unknown action: %ld\n",
>> -					__func__, mem, action, action);
>> +					__func__, mbs, action, action);
>>  			ret = -EINVAL;
>>  	}
>>
>> @@ -238,19 +252,34 @@
>>  static int memory_block_change_state(struct memory_block *mem,
>>  		unsigned long to_state, unsigned long from_state_req)
>>  {
>> +	struct memory_block_section *mbs;
>>  	int ret = 0;
>> +
>>  	mutex_lock(&mem->state_mutex);
>>
>> -	if (mem->state != from_state_req) {
>> -		ret = -EINVAL;
>> -		goto out;
>> +	list_for_each_entry(mbs, &mem->sections, next) {
>> +		if (mbs->state != from_state_req)
>> +			continue;
>> +
>> +		ret = memory_block_action(mbs, to_state);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	if (ret) {
>> +		list_for_each_entry(mbs, &mem->sections, next) {
>> +			if (mbs->state == from_state_req)
>> +				continue;
>> +
>> +			if (memory_block_action(mbs, to_state))
>> +				printk(KERN_ERR "Could not re-enable memory "
>> +				       "section %lx\n", mbs->phys_index);
>> +		}
>>  	}
> 
> Please just use a goto here.  It's nicer looking, and much more in line
> with what's there already.

Not sure if I follow on where you want the goto.  If you mean after the
if (memory_block_action())...  I purposely did not have a goto here.
Since this is in the recovery path I wanted to make sure we tried to return
every memory section to the original state.

> 
> ...
>> ===================================================================
>> --- linux-2.6.orig/include/linux/memory.h	2010-07-15 08:48:41.000000000 -0500
>> +++ linux-2.6/include/linux/memory.h	2010-07-15 09:54:06.000000000 -0500
>> @@ -19,9 +19,15 @@
>>  #include <linux/node.h>
>>  #include <linux/compiler.h>
>>  #include <linux/mutex.h>
>> +#include <linux/list.h>
>>
>> -struct memory_block {
>> +struct memory_block_section {
>> +	unsigned long state;
>>  	unsigned long phys_index;
>> +	struct list_head next;
>> +};
>> +
>> +struct memory_block {
>>  	unsigned long state;
>>  	/*
>>  	 * This serializes all state change requests.  It isn't
>> @@ -34,6 +40,7 @@
>>  	void *hw;			/* optional pointer to fw/hw data */
>>  	int (*phys_callback)(struct memory_block *);
>>  	struct sys_device sysdev;
>> +	struct list_head sections;
>>  };
> 
> It looks like we have state in both the memory_block and
> memory_block_section.  That seems a bit confusing to me.  This also
> looks like it would permit non-contiguous memory_block_sections in a
> memory_block.  Is that what you intended?

The two state fields are a bit confusing, perhaps slightly different
names, block_state and section_state.  The reason for two state fileds
is that memory online/offline is done on a memory block and an entire
memory block is considered online or offline.  The state field in the
memory_block_section is used to track the state of each of the memory
sections as you work through onlining or offlining the entire block
so that if an error occurs we can return each memory section to its 
original state.

You're correct that there is nothing that prevents non-contiguous memory
sections in a memory block.  It was not my intention to have this, but
looking at the patches there is nothing to prevent it.

> 
> If the memory_block's state was inferred to be the same as each
> memory_block_section, couldn't we just keep a start and end phys_index
> in the memory_block, and get away from having memory_block_sections at
> all?

Oooohhh... I like.  Looking at the code it appears this is possible.  I'll
try this out and include it in the next version of the patch.

Do you think we need to add an additional file to each memory block directory
to indicate the number of memory sections in the memory block that are actually
present?

-Nathan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>