double-check me?

double-check me?

[Jeff Garzik]

Maybe you guys can spot something I'm missing here.  alloc_netdev is 
supposed to guarantee that dev->priv is aligned by 32 bytes:

struct net_device *alloc_netdev(int sizeof_priv, const char *mask,
                                        void (*setup)(struct net_device *))
{
         struct net_device *dev;
         int alloc_size;

         /* ensure 32-byte alignment of the private area */
         alloc_size = sizeof (*dev) + sizeof_priv + 31;

         dev = (struct net_device *) kmalloc (alloc_size, GFP_KERNEL);
         if (dev == NULL)
         {
                 printk(KERN_ERR "alloc_dev: Unable to allocate device 
memory.\n"
);
                 return NULL;
         }

         memset(dev, 0, alloc_size);

         if (sizeof_priv)
                 dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);


Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?

It seems to work 2.[456] for a long time, so I am doubting myself... 
surely it would have caused memory corruption or something by now if I 
have really found a bug.

	Jeff

Re: double-check me?

[Krzysztof Halasa]

Jeff Garzik <jgarzik@pobox.com> writes:

> Maybe you guys can spot something I'm missing here.  alloc_netdev is
> supposed to guarantee that dev->priv is aligned by 32 bytes:
> 
>          struct net_device *dev;
> 
>          if (sizeof_priv)
>                  dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);
> 
> 
> Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?
> 
> It seems to work 2.[456] for a long time, so I am doubting
> myself... surely it would have caused memory corruption or something
> by now if I have really found a bug.

Looks ok... not very readable, though.

dev + 1 = ((u8*)dev) + sizeof(dev) = pointer to end of net_device struct.

(X + 31) & ~31 makes sure X is 32-bytes aligned (0-31 bytes are added).
Hope the alloc_size has enough space for this.

31 should be better 0x1F I think.

I also like sizeof(struct net_device) more than sizeof(*dev).

Why do you want "+ 1" if you add sizeof(*dev)?
-- 
Krzysztof Halasa
Network Administrator

Re: double-check me?

[Jason Lunz]

jgarzik@pobox.com said:
>          if (sizeof_priv)
>                  dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);
> 
> 
> Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?

are you missing that the "dev + 1" pointer arithmetic is already adding
sizeof(*dev) to dev, rather than just 1 byte? "(dev + 1)" is a pointer
to the private area after the actual struct net_device, and
"((long)(dev + 1) + 31)" adds 31 bytes of padding. The final "& ~31"
chops off any excess padding from the 31 that was added and actually
aligns the pointer.

Seems right to me, but I'm not used to playing alignment tricks.

Jason

Re: double-check me?

[Jeff Garzik]

Krzysztof Halasa wrote:
> Jeff Garzik <jgarzik@pobox.com> writes:
> 
> 
>>Maybe you guys can spot something I'm missing here.  alloc_netdev is
>>supposed to guarantee that dev->priv is aligned by 32 bytes:
>>
>>         struct net_device *dev;
>>
>>         if (sizeof_priv)
>>                 dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);
>>
>>
>>Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?
>>
>>It seems to work 2.[456] for a long time, so I am doubting
>>myself... surely it would have caused memory corruption or something
>>by now if I have really found a bug.
> 
> 
> Looks ok... not very readable, though.
> 
> dev + 1 = ((u8*)dev) + sizeof(dev) = pointer to end of net_device struct.


C pointer arithmatic.  Ok, duh, thanks :)

	Jeff, too used to void pointer arith

Re: double-check me?

[Jason Lunz]

jgarzik@pobox.com said:
> C pointer arithmatic.  Ok, duh, thanks :)
> 	Jeff, too used to void pointer arith

Don't get _too_ used to it. arithmetic on (void *) is a gcc extension.
it's undefined in ansi C.

Jason

Re: double-check me?

[Jeff Garzik]

On Mon, Aug 18, 2003 at 02:38:50PM +0000, Jason Lunz wrote:
> jgarzik@pobox.com said:
> > C pointer arithmatic.  Ok, duh, thanks :)
> > 	Jeff, too used to void pointer arith
> 
> Don't get _too_ used to it. arithmetic on (void *) is a gcc extension.
> it's undefined in ansi C.

Yes, I know.

It's used extensively in the kernel, however.

	Jeff