On Mon, 2019-05-06 at 12:32 +0200, Ilya Dryomov wrote: > On Thu, May 2, 2019 at 8:46 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > GCC9 is throwing a lot of warnings about unaligned access. This patch > > fixes some of them by changing most of the sockaddr handling functions > > to take a pointer to struct ceph_entity_addr instead of struct > > sockaddr_storage. The lower functions can then take copies or do > > unaligned accesses as needed. > > Linus has disabled this warning in 5.1 [1], but these look real to me, > at least when sockaddr_storage is coming from ceph_entity_inst. I'd be > happier if we defined non-packed variants of ceph_entity_addr and > ceph_entity_inst and used them throughout the code, but that's likely > a lot more churn. I might take a stab at it just to see how it goes... > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f303d60534c46aa1a239f29c321f95c83dda748 > Yeah, they looked like legit problems to me too. I too was going to move them over to use a non-packed representation internally, but it did turn out to be quite a bit of churn and copying. YMMV of course. > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > --- > > net/ceph/messenger.c | 77 ++++++++++++++++++++++---------------------- > > 1 file changed, 38 insertions(+), 39 deletions(-) > > > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c > > index 3083988ce729..54713836cac3 100644 > > --- a/net/ceph/messenger.c > > +++ b/net/ceph/messenger.c > > @@ -449,7 +449,7 @@ static void set_sock_callbacks(struct socket *sock, > > */ > > static int ceph_tcp_connect(struct ceph_connection *con) > > { > > - struct sockaddr_storage *paddr = &con->peer_addr.in_addr; > > + struct sockaddr_storage addr = con->peer_addr.in_addr; > > Probably worth a comment, even something as short as /* align */. > Sounds good. I'll add those to the ones below too. > > struct socket *sock; > > unsigned int noio_flag; > > int ret; > > @@ -458,7 +458,7 @@ static int ceph_tcp_connect(struct ceph_connection *con) > > > > /* sock_create_kern() allocates with GFP_KERNEL */ > > noio_flag = memalloc_noio_save(); > > - ret = sock_create_kern(read_pnet(&con->msgr->net), paddr->ss_family, > > + ret = sock_create_kern(read_pnet(&con->msgr->net), addr.ss_family, > > SOCK_STREAM, IPPROTO_TCP, &sock); > > memalloc_noio_restore(noio_flag); > > if (ret) > > @@ -474,7 +474,7 @@ static int ceph_tcp_connect(struct ceph_connection *con) > > dout("connect %s\n", ceph_pr_addr(&con->peer_addr.in_addr)); > > > > con_sock_state_connecting(con); > > - ret = sock->ops->connect(sock, (struct sockaddr *)paddr, sizeof(*paddr), > > + ret = sock->ops->connect(sock, (struct sockaddr *)&addr, sizeof(addr), > > O_NONBLOCK); > > if (ret == -EINPROGRESS) { > > dout("connect %s EINPROGRESS sk_state = %u\n", > > @@ -1795,12 +1795,13 @@ static int verify_hello(struct ceph_connection *con) > > return 0; > > } > > > > -static bool addr_is_blank(struct sockaddr_storage *ss) > > +static bool addr_is_blank(struct ceph_entity_addr *ea) > > { > > - struct in_addr *addr = &((struct sockaddr_in *)ss)->sin_addr; > > - struct in6_addr *addr6 = &((struct sockaddr_in6 *)ss)->sin6_addr; > > + struct sockaddr_storage ss = ea->in_addr; > > Same here. > > > + struct in_addr *addr = &((struct sockaddr_in *)&ss)->sin_addr; > > + struct in6_addr *addr6 = &((struct sockaddr_in6 *)&ss)->sin6_addr; > > > > - switch (ss->ss_family) { > > + switch (ss.ss_family) { > > case AF_INET: > > return addr->s_addr == htonl(INADDR_ANY); > > case AF_INET6: > > @@ -1810,25 +1811,27 @@ static bool addr_is_blank(struct sockaddr_storage *ss) > > } > > } > > > > -static int addr_port(struct sockaddr_storage *ss) > > +static int addr_port(struct ceph_entity_addr *ea) > > { > > - switch (ss->ss_family) { > > + struct sockaddr_storage ss = ea->in_addr; > > + > > + switch (ss.ss_family) { > > case AF_INET: > > - return ntohs(((struct sockaddr_in *)ss)->sin_port); > > + return ntohs(((struct sockaddr_in *)&ss)->sin_port); > > case AF_INET6: > > - return ntohs(((struct sockaddr_in6 *)ss)->sin6_port); > > + return ntohs(((struct sockaddr_in6 *)&ss)->sin6_port); > > } > > return 0; > > } > > Can we do the get_unaligned() dance instead of copying here? > I tried that a couple of different ways last week, but we have to take a pointer to the sockaddr_storage in order to cast to it to a sockaddr_in{6}. Maybe I'm missing something though, so let me know if you see a better way to do this. That said, we only call this from process_banner(), so I don't think the copying is likely to harm performance. -- Jeff Layton <jlayton@xxxxxxxxxx>
