Date: Tue, 9 Feb 2010 19:21:14 +0100 From: Luigi Rizzo <rizzo@iet.unipi.it> To: "M. Warner Losh" <imp@bsdimp.com> Cc: net@freebsd.org Subject: Re: struct sockaddr * and alignment Message-ID: <20100209182114.GD89720@onelab2.iet.unipi.it> In-Reply-To: <20100209.103411.506052712871889475.imp@bsdimp.com> References: <20100209.103411.506052712871889475.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 09, 2010 at 10:34:11AM -0700, M. Warner Losh wrote:
> Greetings,
>
> I've found a few inconsistencies in the sockaddr stuff in the tree.
> I'm not sure where to go to get a definitive answer, so I thought I'd
> start here.
>
> I got here looking at the recent wake breakage on mips. It turns out
> that the warning was:
>
> src/usr.sbin/wake/wake.c: In function 'find_ether':
> src/usr.sbin/wake/wake.c:123: warning: cast increases required alignment of target type
>
> which comes from
> sdl = (struct sockaddr_dl *)ifa->ifa_addr;
>
> The problem is that on MIPS struct sockaddr * is byte aligned and
> sockaddr_dl * is word aligned, so the compiler is rightly telling us
> that there might be a problem here.
>
> However, further digging shows that there will never be a problem
> here with alignment. struct sockaddr_storage has a int64 in it to
> force it to be __aligned(8). So I thought to myself "why don't I just
> add __aligned(8) to the struct sockaddr definition?" After all, the
> kernel goes to great lengths to return data so aligned, and user code
> also keeps things aligned.
>
> Sure enough, that fixes this warning. Yea. But, sadly, it causes
> other problems. If you look at sbin/atm/atmconfig/natm.c you'll see
> code like:
>
> static void
> store_route(struct rt_msghdr *rtm)
> {
> ...
> char *cp
> struct sockaddr *sa;
> ...
>
> cp = (char *)(rtm + 1);
> ...
> sa = (struct sockaddr *)cp;
> cp += roundup(sa->sa_len, sizeof(long));
> ...
>
> which breaks because we're now casting from an __aligned(1) char * to
> an __aligned(8) sockaddr *.
>
> And it is only rounding the size of the structure to long, rather than
> int64 like sockaddr_storage suggests is the proper alignment. But I
> haven't looked in the kernel to see if there's an issue there with
> routing sockets or not.
>
> The other extreme is to put __aligned(1) on all the sockaddr_foo
> structures. This would solve the compiler warning, but would have a
> negative effect on performance in accessing these elements (because
> the compiler would have to generate calls to bcopy or equivalent to
> access the scalar members that are larger than a byte). This cure
> would be worse than the disease.
>
> So the question here is "What is the right solution here?" It has me
> stumped. So I dropped WARNS level down from 6 to 3 for wake.c.
There does not seem to be a good fix for this type of problems
(copying structures with >1 alignment requirements from/to
linearized messages) that does not involve a bcopy.
I think struct sockaddr (the generic versions) are normally not used
in speed-critical paths, so having a bcopy inserted by the compiler
(is this what really happens ?) should not harm too much.
On the other hand, maybe the places where the misalignment occurs
are not too many (and typically occur across ioctl/sockopt or similar
system calls ?) so it might be preferable to expose the alignment
requirements and manually fix the offending code. Many of those
places might be already correct.
cheers
luigi
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100209182114.GD89720>