Re: [Pcihpd-discuss] [PATCH 2/5] Construct one fakephp slot per pci slot

[Rolf Eike Beer]

Am Mittwoch, 14. November 2007 schrieb Alex Chiang:
> Hi Eike,
>
> * Rolf Eike Beer <eike-hotplug@xxxxxxxxx>:
> > Alex Chiang wrote:
> > > --- a/drivers/pci/hotplug/fakephp.c
> > > +++ b/drivers/pci/hotplug/fakephp.c
> > > @@ -93,6 +93,7 @@ static int add_slot(struct pci_dev *dev)
> > >  	struct dummy_slot *dslot;
> > >  	struct hotplug_slot *slot;
> > >  	int retval = -ENOMEM;
> > > +	static int count = 1;
> > >
> > >  	slot = kzalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
> > >  	if (!slot)
> > > @@ -106,7 +107,8 @@ static int add_slot(struct pci_dev *dev)
> > >  	slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
> > >  	slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
> > >
> > > -	slot->name = &dev->dev.bus_id[0];
> > > +	slot->name = kmalloc(8, GFP_KERNEL);
> > > +	sprintf(slot->name, "fake%d", count++);
> > >  	dbg("slot->name = %s\n", slot->name);
> > >
> > >  	dslot = kmalloc(sizeof(struct dummy_slot), GFP_KERNEL);
> >
> > This is ugly. Please do it the way we already do e.g. for
> > acpiphp: add a char[8] to "struct dummy_slot" and just
> > reference that here.
>
> I took at look at the code in acpiphp you're talking about, and
> I'm not sure why you think the above is "ugly". We're talking
> about a runtime vs compile time storage for the name, and doing a
> kmalloc/sprintf is a pretty standard idiom.
>
> BTW, I did incorporate both Linas' and Willy's comments, by
> changing the kmalloc size to an explicit 32, and using snprintf
> instead.
>
> In any case, for your particular comment, I think I'm going to
> leave it alone for now, and let Greg weigh in with the final
> recommendation.

Because we have another allocation of very small size for every slot here.

struct dummy_slot has a size of 4 pointers, that's 16 byte for 32 bit 
architectures. Putting 8 byte of additional storage here would save a 
complete allocation on 32 bit platforms. For 64 bit platforms the memory 
usage is the same but we do less allocations (i.e. less points to fail) and 
less memory fragmentation.

Btw: your code lacks a check if kmalloc() returns NULL.

Eike

Attachment: signature.asc
Description: This is a digitally signed message part.