[v2,1/1] IB/mlx4: Unaligned access in send_reply_to_slave

Commit Message

Shamir Rabinovitch May 17, 2016, 5:16 a.m. UTC

From: Shamir Rabinovitch <[e-mail]>

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 <[e-mail]>
---
 drivers/infiniband/hw/mlx4/mcg.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Comments

Leon Romanovsky May 17, 2016, 7:39 a.m. UTC | #1

On Tue, May 17, 2016 at 01:16:38AM -0400, [e-mail] wrote:
> From: Shamir Rabinovitch <[e-mail]>
> 
> 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 <[e-mail]>

The difference between this version and previous version is in the
title, am I right?

Thanks.

Shamir Rabinovitch May 17, 2016, 8:37 a.m. UTC | #2

On Tue, May 17, 2016 at 10:39:40AM +0300, Leon Romanovsky wrote:
> On Tue, May 17, 2016 at 01:16:38AM -0400, [e-mail] wrote:
> > From: Shamir Rabinovitch <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe May 17, 2016, 5:07 p.m. UTC | #3

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shamir Rabinovitch May 18, 2016, 8:39 a.m. UTC | #4

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);