Message ID | 1463462198-15674-1-git-send-email-shamir.rabinovitch@oracle.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
On Tue, May 17, 2016 at 01:16:38AM -0400, shamir.rabinovitch@oracle.com wrote: > From: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > The problem is that the function 'send_reply_to_slave' get the > 'req_sa_mad' as pointer whose address can be unaligned to 8 bytes. > In this case the compiler cannot know in advance what will be the > alignment of the 'data' field. > > Sowmini Varadhan pointed to this reply from Dave Miller that say > that memcpy should not be used to solve alignment issues: > https://lkml.org/lkml/2015/10/21/352 > > The reason why memcpy works here is because we memcpy someting that > is bigger then 8 bytes and so the compiler cannot optimize this to > 'ldx' instruction. > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> The difference between this version and previous version is in the title, am I right? Thanks.
On Tue, May 17, 2016 at 10:39:40AM +0300, Leon Romanovsky wrote: > On Tue, May 17, 2016 at 01:16:38AM -0400, shamir.rabinovitch@oracle.com wrote: > > From: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > > > The problem is that the function 'send_reply_to_slave' get the > > 'req_sa_mad' as pointer whose address can be unaligned to 8 bytes. > > In this case the compiler cannot know in advance what will be the > > alignment of the 'data' field. > > > > Sowmini Varadhan pointed to this reply from Dave Miller that say > > that memcpy should not be used to solve alignment issues: > > https://lkml.org/lkml/2015/10/21/352 > > > > The reason why memcpy works here is because we memcpy someting that > > is bigger then 8 bytes and so the compiler cannot optimize this to > > 'ldx' instruction. > > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > The difference between this version and previous version is in the > title, am I right? Correct. Minor request from Or Gerlitz. Main issue which I want to address is this: In the CM & MAD code we see many issues of unaligned access. This has 2 main sources. 1. CM & MAD packets that are not built according to the alignment rules 2. Aligned structures that are put in arbitrary unaligned memory locations This issue affect platforms that require aligned access to anything above 1 byte (e.g. sparc64). The best way to avoid this issue is to design the packed wire packets with alignment rules in place and align the start of any such structure to 8 bytes when used. Given that this is not the case in CM & MAD and it's wire protocol I try to find the best way to avoid such issues w/o changing the whole code. As Dave Miller noted in another case - the compiler is free to look in to the cast and if it see that the object we try to copy has size of 8 bytes it can emit the memcpy and change it to 'ldx' instruction. 'ldx' instruction is sensitive to 8 bytes alignment and so we have the issue. Use of 'get_unaligned' should be option 1 to consider. Only issue with this macro is that it only take care for data types with size of 8, 16, 32, 64 bit. It is good when we try to access something that is one of those types. But when we try to cast some bigger structure and then copy some enum from this structure it is not clear if this is the best way. This is what I try to review here. Comments are more then welcome.. > > Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2016 at 11:37:11AM +0300, Shamir Rabinovitch wrote:
> Comments are more then welcome..
Something like this should fix it with less overhead:
struct ib_sa_mcmember_data {
[..]
} __packed __aligned(4);
Which tells the compiler the structure is only aligned on dword
boundaries. It will then emit the proper architecture dependent
unaligned read for the 8 byte values.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2016 at 11:07:33AM -0600, Jason Gunthorpe wrote: > On Tue, May 17, 2016 at 11:37:11AM +0300, Shamir Rabinovitch wrote: > > > Comments are more then welcome.. > > Something like this should fix it with less overhead: > > struct ib_sa_mcmember_data { > [..] > } __packed __aligned(4); > > Which tells the compiler the structure is only aligned on dword > boundaries. It will then emit the proper architecture dependent > unaligned read for the 8 byte values. > > Jason Thanks Jason. I considered to un-optimize this memcp line but your idea is more clean. Gave quick look on the assembly and it seems that you are correct. I could not find the 'ldx' around the location where I had the memcpy. I do full test with this kernel hope it could confirm the fix. If all well I will re post the updated patch to mailing list. BR, Shamir -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/mlx4/mcg.c b/drivers/infiniband/hw/mlx4/mcg.c index 99451d8..180f76b 100644 --- a/drivers/infiniband/hw/mlx4/mcg.c +++ b/drivers/infiniband/hw/mlx4/mcg.c @@ -319,8 +319,9 @@ static int send_reply_to_slave(int slave, struct mcast_group *group, struct ib_sa_mad *req_sa_mad, u16 status) { struct ib_sa_mad mad; + struct ib_sa_mcmember_data req_data; struct ib_sa_mcmember_data *sa_data = (struct ib_sa_mcmember_data *)&mad.data; - struct ib_sa_mcmember_data *req_sa_data = (struct ib_sa_mcmember_data *)&req_sa_mad->data; + struct ib_sa_mcmember_data *req_sa_data = &req_data; int ret; memset(&mad, 0, sizeof mad); @@ -343,6 +344,11 @@ static int send_reply_to_slave(int slave, struct mcast_group *group, /* reconstruct VF's requested join_state and port_gid */ sa_data->scope_join_state &= 0xf0; sa_data->scope_join_state |= (group->func[slave].join_state & 0x0f); + BUILD_BUG_ON(sizeof(req_sa_mad->data) < sizeof(req_data)); + /* req_sa_mad is packed structure whose start address is not + * aligned to 8 + */ + memcpy(&req_data, &req_sa_mad->data, sizeof(req_data)); memcpy(&sa_data->port_gid, &req_sa_data->port_gid, sizeof req_sa_data->port_gid); ret = send_mad_to_slave(slave, group->demux, (struct ib_mad *)&mad);