RE: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

RE: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

[Tolentino, Matthew E]

>What does this end up naming the module?  If it ends up being memory.o,
>you might want to think about making it something a bit more
>descriptive.  Does it even need to be allowed to be a module, 
>or is that
>standard ACPI practice?

Currently, it names it memory.ko.  Len has also suggested that it be
something more like memory_hotplug.ko or memory_hp.ko or something
like that, because the ACPI spec does include verbage about the
system memory map...  As this driver does handle "memory"
devices, it kinda makes sense, but we're not wed to any particular 
name...  ;-)  

I would prefer to maintain the option to build this driver as a module 
at least until there is more testing.  It's certainly easier to work 
with right now as a module.

>I think I'd slightly prefer something that looks more like this (same
>thing goes for the add function): 
>
>acpi_memory_disable_device(struct acpi_memory_device *mem_device) 
>{
>	int err;
>	u64 start = mem_device->start_addr;
>	u64 len = mem_device->end_addr - start + 1;
>	unsigned long attr = mem_device->read_write_attribute;
>
>	ACPI_FUNCTION_TRACE("acpi_memory_disable_device");
>
>	/* 
>	 * Ask the VM to offline this memory range.
>	 */
>	err = remove_memory(start, len, attr);
>
>	if (err) {
>		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Hot-Remove 
>failed.\n"));
>		return_VALUE(err);	
>	}
>...

Yeah, that's a little easier to read...  

>Other than the add/remove_memory() calls does this have any real
>interaction with any non-ACPI code that I overlooked?

Nope, the only real interaction with the VM is isolated to those 
calls.  This driver is really only for handling the interaction 
with real hardware via ACPI, so it shouldn't need to muck around 
with other code...  

Btw, Keith please give this a once over as I know we talked about
this before...

matt


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php

RE: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

[Dave Hansen]

On Thu, 2004-09-09 at 11:28, Tolentino, Matthew E wrote:
> >What does this end up naming the module?  If it ends up being memory.o,
> >you might want to think about making it something a bit more
> >descriptive.  Does it even need to be allowed to be a module, 
> >or is that
> >standard ACPI practice?
> 
> Currently, it names it memory.ko.  Len has also suggested that it be
> something more like memory_hotplug.ko or memory_hp.ko or something
> like that, because the ACPI spec does include verbage about the
> system memory map...  As this driver does handle "memory"
> devices, it kinda makes sense, but we're not wed to any particular 
> name...  ;-)  

How about including acpi in the name?  acpi_memory.o? 
acpi_memhotplug.o?

> >Other than the add/remove_memory() calls does this have any real
> >interaction with any non-ACPI code that I overlooked?
> 
> Nope, the only real interaction with the VM is isolated to those 
> calls.  This driver is really only for handling the interaction 
> with real hardware via ACPI, so it shouldn't need to muck around 
> with other code...  

Cool.  Looks pretty good.

-- Dave



-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM. 
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php

RE: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

[S, Naveen B]

> 
> >I think I'd slightly prefer something that looks more like this (same
> >thing goes for the add function):
> >
> >acpi_memory_disable_device(struct acpi_memory_device *mem_device)
> >{
> >	int err;
> >	u64 start = mem_device->start_addr;
> >	u64 len = mem_device->end_addr - start + 1;
> >	unsigned long attr = mem_device->read_write_attribute;
> >
> >	ACPI_FUNCTION_TRACE("acpi_memory_disable_device");
> >
> >	/*
> >	 * Ask the VM to offline this memory range.
> >	 */
> >	err = remove_memory(start, len, attr);
> >
> >	if (err) {
> >		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Hot-Remove
> >failed.\n"));
> >		return_VALUE(err);
> >	}
> >...
> 
> Yeah, that's a little easier to read...
> 
This looks good and easier to read. Thanks for the suggestion, Dave.

--Naveen


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php

RE: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

[S, Naveen B]

> > +	if (remove_memory(mem_device->start_addr,
> > +		(mem_device->end_addr - mem_device->start_addr) + 1,
> > +		mem_device->read_write_attribute)) {
> > +		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Hot-Remove
failed.\n"));
> > +		return_VALUE(-EINVAL);
> > +	}
> > +
> 
> How about actually passing up the value that remove_memory() gave
back?
> Also, what's with the +1 on the size?

To find the actual length of memory being added/removed. This is to
include start and end byte in the given range.

> 
> If it ends up that all of the callers of remove_memory() prefer to
give
> start and end addresses, we'll probably change the calling convention
at
> some point.  Whatever is most natural.

Looks good. This should avoid usage of +1.

--Naveen




-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php

Re: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

[Bjorn Helgaas]

On Friday 10 September 2004 7:01 am, S, Naveen B wrote:
> > If it ends up that all of the callers of remove_memory() prefer to give
> > start and end addresses, we'll probably change the calling convention at
> > some point.  Whatever is most natural.
> 
> Looks good. This should avoid usage of +1.

IMHO an "address, size" convention is much nicer than "start, end".

I don't like "start, end" because (1) there's always the ambiguity of
whether the last byte is included, and (2) zero-length ranges look funny
(e.g., "0x0,0xffff").

"address, size" avoids both problems, and is already used by tons of
kernel interfaces like ioremap(), the dma interfaces, verify_area(),
memset(), etc, etc.


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php

RE: [Lhms-devel] [PATCH] ACPI based Memory Hotplug DriverPatch

[S, Naveen B]

> IMHO an "address, size" convention is much nicer than "start, end".
> 
> I don't like "start, end" because (1) there's always the ambiguity of
> whether the last byte is included, and (2) zero-length ranges look
funny
> (e.g., "0x0,0xffff").
> 
> "address, size" avoids both problems, and is already used by tons of
> kernel interfaces like ioremap(), the dma interfaces, verify_area(),
> memset(), etc, etc.

I agree, Thanks Bjorn.


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php