Prev: v3 Update the find_memory_block declaration
Next: ds2782_battery: Rename get_current and get_voltage operations
From: KAMEZAWA Hiroyuki on 20 Jul 2010 03:30
On Mon, 19 Jul 2010 22:55:08 -0500
Nathan Fontenot <nfont(a)austin.ibm.com> wrote:

> Update the memory sysfs code that each sysfs memory directory is now
> considered a memory block that can contain multiple memory sections per
> memory block. The default size of each memory block is SECTION_SIZE_BITS
> to maintain the current behavior of having a single memory section per
> memory block (i.e. one sysfs directory per memory section).
>
> For architectures that want to have memory blocks span multiple
> memory sections they need only define their own memory_block_size_bytes()
> routine.
>
> Signed-off-by: Nathan Fontenot <nfont(a)austin.ibm.com>
> ---
> drivers/base/memory.c | 141 ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 98 insertions(+), 43 deletions(-)
>
> Index: linux-2.6/drivers/base/memory.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/memory.c 2010-07-19 20:44:01.000000000 -0500
> +++ linux-2.6/drivers/base/memory.c 2010-07-19 21:12:22.000000000 -0500
> @@ -28,6 +28,14 @@
> #include <asm/uaccess.h>
>
> #define MEMORY_CLASS_NAME "memory"
> +#define MIN_MEMORY_BLOCK_SIZE (1 << SECTION_SIZE_BITS)
> +
> +static int sections_per_block;
> +
> +static inline int base_memory_block_id(int section_nr)
> +{
> + return (section_nr / sections_per_block) * sections_per_block;
> +}
>
> static struct sysdev_class memory_sysdev_class = {
> .name = MEMORY_CLASS_NAME,
> @@ -82,22 +90,21 @@ EXPORT_SYMBOL(unregister_memory_isolate_
> * register_memory - Setup a sysfs device for a memory block
> */
> static
> -int register_memory(struct memory_block *memory, struct mem_section *section)
> +int register_memory(struct memory_block *memory)
> {
> int error;
>
> memory->sysdev.cls = &memory_sysdev_class;
> - memory->sysdev.id = __section_nr(section);
> + memory->sysdev.id = memory->start_phys_index;

I'm curious that this memory->start_phys_index can't overflow ?
sysdev.id is 32bit.


Thanks,
-Kame

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dave Hansen on 20 Jul 2010 15:20
On Mon, 2010-07-19 at 22:55 -0500, Nathan Fontenot wrote:
> +static int add_memory_section(int nid, struct mem_section *section,
> + unsigned long state, enum mem_add_context context)
> +{
> + struct memory_block *mem;
> + int ret = 0;
> +
> + mem = find_memory_block(section);
> + if (mem) {
> + atomic_inc(&mem->section_count);
> + kobject_put(&mem->sysdev.kobj);
> + } else
> + ret = init_memory_block(&mem, section, state);
> +
> if (!ret) {
> - if (context == HOTPLUG)
> + if (context == HOTPLUG &&
> + atomic_read(&mem->section_count) == sections_per_block)
> ret = register_mem_sect_under_node(mem, nid);
> }

I think the atomic_inc() can race with the atomic_dec_and_test() in
remove_memory_block().

Thread 1 does:

mem = find_memory_block(section);

Thread 2 does

atomic_dec_and_test(&mem->section_count);

and destroys the memory block, Thread 1 runs again:

if (mem) {
atomic_inc(&mem->section_count);
kobject_put(&mem->sysdev.kobj);
} else

but now mem got destroyed by Thread 2. You probably need to change
find_memory_block() to itself take a reference, and to use
atomic_inc_unless().

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dave Hansen on 20 Jul 2010 15:30
On Mon, 2010-07-19 at 22:55 -0500, Nathan Fontenot wrote:
> +static u32 get_memory_block_size(void)
> +{
> + u32 block_sz;
> +
> + block_sz = memory_block_size_bytes();
> +
> + /* Validate blk_sz is a power of 2 and not less than section size */
> + if ((block_sz & (block_sz - 1)) || (block_sz < MIN_MEMORY_BLOCK_SIZE))
> + block_sz = MIN_MEMORY_BLOCK_SIZE;

Is this worth a WARN_ON()? Seems pretty bogus if someone is returning
funky block sizes.

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dave Hansen on 26 Jul 2010 22:40
On Fri, 2010-07-23 at 22:09 -0500, Nathan Fontenot wrote:
> If we add a lock should I submit it as part of this patchset? or
> submit it
> as a follow-on?

It should probably be at the beginning of the patch set. We don't want
to have a case where your set introduces races that we _need_ a later
patch to fix.

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: v3 Update the find_memory_block declaration
Next: ds2782_battery: Rename get_current and get_voltage operations