| Message ID | 1430448168-38479-1-git-send-email-david.ahern@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, Le jeudi 30 avril 2015 à 22:42 -0400, David Ahern a écrit : > Addresses the following kernel logs seen during boot of sparc systems: > > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > > Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason > Gunthorpe. > Signed-off-by: David Ahern <david.ahern@oracle.com> > --- > v2 > - updated per Jason's comments > > drivers/infiniband/core/cm.c | 17 ++++++++--------- > include/rdma/ib_cm.h | 4 ++-- > 2 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index e28a494e2a3a..cbbf741987b3 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -437,20 +437,19 @@ static struct cm_id_private * cm_acquire_id(__be32 local_id, __be32 remote_id) > return cm_id_priv; > } > > -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) > +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask) > { > int i; > > - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) > - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & > - ((unsigned long *) mask)[i]; > + for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++) > + dst[i] = src[i] & mask[i]; > } > > static int cm_compare_data(struct ib_cm_compare_data *src_data, > struct ib_cm_compare_data *dst_data) > { > - u8 src[IB_CM_COMPARE_SIZE]; > - u8 dst[IB_CM_COMPARE_SIZE]; > + u32 src[IB_CM_COMPARE_SIZE]; > + u32 dst[IB_CM_COMPARE_SIZE]; > You have to change IB_CM_COMPARE_SIZE, as it's going to allocate more bytes than necessary. Perhaps renaming IB_CM_COMPARE_SIZE to IB_CM_COMPARE_COUNT would be good, then remove the division by sizeof(u32). > if (!src_data || !dst_data) > return 0; > @@ -460,10 +459,10 @@ static int cm_compare_data(struct ib_cm_compare_data *src_data, > return memcmp(src, dst, IB_CM_COMPARE_SIZE); > } > > -static int cm_compare_private_data(u8 *private_data, > +static int cm_compare_private_data(u32 *private_data, > struct ib_cm_compare_data *dst_data) > { > - u8 src[IB_CM_COMPARE_SIZE]; > + u32 src[IB_CM_COMPARE_SIZE]; > > if (!dst_data) > return 0; > @@ -546,7 +545,7 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device, > > while (node) { > cm_id_priv = rb_entry(node, struct cm_id_private, service_node); > - data_cmp = cm_compare_private_data(private_data, > + data_cmp = cm_compare_private_data((u32 *) private_data, > cm_id_priv->compare_data); > if ((cm_id_priv->id.service_mask & service_id) == > cm_id_priv->id.service_id && > diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h > index 0e3ff30647d5..0dd576f40c83 100644 > --- a/include/rdma/ib_cm.h > +++ b/include/rdma/ib_cm.h > @@ -337,8 +337,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id); > #define IB_SDP_SERVICE_ID_MASK cpu_to_be64(0xFFFFFFFFFFFF0000ULL) > > struct ib_cm_compare_data { > - u8 data[IB_CM_COMPARE_SIZE]; > - u8 mask[IB_CM_COMPARE_SIZE]; > + u32 data[IB_CM_COMPARE_SIZE]; > + u32 mask[IB_CM_COMPARE_SIZE]; > }; > > /** Anyway, you might want to compile this part of the kernel with -Wcast-align and ignore the false positive (in particular every other use of container_of()). Regards.
In addition to Yann's comments: On 05/01/15 04:42, David Ahern wrote: > -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) > +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask) Please consider to constify the src and mask arguments. > - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) > - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & > - ((unsigned long *) mask)[i]; > + for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++) > + dst[i] = src[i] & mask[i]; A single space after the equals sign is sufficient. Thanks, Bart. -- 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 5/1/15 2:45 AM, Yann Droneaud wrote: >> static int cm_compare_data(struct ib_cm_compare_data *src_data, >> struct ib_cm_compare_data *dst_data) >> { >> - u8 src[IB_CM_COMPARE_SIZE]; >> - u8 dst[IB_CM_COMPARE_SIZE]; >> + u32 src[IB_CM_COMPARE_SIZE]; >> + u32 dst[IB_CM_COMPARE_SIZE]; >> > > You have to change IB_CM_COMPARE_SIZE, as it's going to allocate more > bytes than necessary. d'oh. yes, oversight. Thanks for pointing out. > > Perhaps renaming IB_CM_COMPARE_SIZE to IB_CM_COMPARE_COUNT would be > good, then remove the division by sizeof(u32). I feel like I stepped into a tar pit... It's a bit odd that IB_CM_COMPARE_SIZE is part of an enum rather than a #define. It seems like the number of bytes being compared is what is relevant here; comparing by u32's or unsigned long's is the optimization. So, I'd prefer to leave it as SIZE. If there are no objections I think the better thing to do is pull it out as: /* compare's are done u32 at a time */ #define IB_CM_COMPARE_SIZE 64/sizeof(u32) >> diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h >> index 0e3ff30647d5..0dd576f40c83 100644 >> --- a/include/rdma/ib_cm.h >> +++ b/include/rdma/ib_cm.h >> @@ -337,8 +337,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id); >> #define IB_SDP_SERVICE_ID_MASK cpu_to_be64(0xFFFFFFFFFFFF0000ULL) >> >> struct ib_cm_compare_data { >> - u8 data[IB_CM_COMPARE_SIZE]; >> - u8 mask[IB_CM_COMPARE_SIZE]; >> + u32 data[IB_CM_COMPARE_SIZE]; >> + u32 mask[IB_CM_COMPARE_SIZE]; >> }; >> >> /** > > Anyway, you might want to compile this part of the kernel with > -Wcast-align and ignore the false positive (in particular > every other use of container_of()). Why? David -- 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 5/1/15 3:50 AM, Bart Van Assche wrote: > In addition to Yann's comments: > > On 05/01/15 04:42, David Ahern wrote: >> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) >> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask) > > Please consider to constify the src and mask arguments. > >> - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) >> - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & >> - ((unsigned long *) mask)[i]; >> + for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++) >> + dst[i] = src[i] & mask[i]; > > A single space after the equals sign is sufficient. > ack -- 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 Thu, Apr 30, 2015 at 10:42:48PM -0400, David Ahern wrote: > Addresses the following kernel logs seen during boot of sparc systems: > > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] > > Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason > Gunthorpe. +1 to everyone elses comments.. > -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) > +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask) masy as well add the const while here > - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) > - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & > - ((unsigned long *) mask)[i]; > + for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++) > + dst[i] = src[i] & mask[i]; for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(*dst); i++) Is a little more idiomatic > - data_cmp = cm_compare_private_data(private_data, > + data_cmp = cm_compare_private_data((u32 *) private_data, > cm_id_priv->compare_data); I'd be happier without this cast, at the worst move it up one more layer, at the best, fix the two impacted structures as well. > I feel like I stepped into a tar pit... It's a bit odd that > IB_CM_COMPARE_SIZE is part of an enum rather than a #define. This is, again, idiomatic. '#define CONSTANT' is often frowned upon, the semantics of enum are much cleaner/safer, eg what you trivially posted: > #define IB_CM_COMPARE_SIZE 64/sizeof(u32) Is wrong, missing brackets. >> Anyway, you might want to compile this part of the kernel with >> -Wcast-align and ignore the false positive (in particular >> every other use of container_of()). > Why? To look for more sketchy things that might impact SPARC, ARM, etc. 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 5/1/15 10:24 AM, Jason Gunthorpe wrote: >> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) >> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask) > > masy as well add the const while here noted by Bart. > >> - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) >> - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & >> - ((unsigned long *) mask)[i]; >> + for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++) >> + dst[i] = src[i] & mask[i]; > > for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(*dst); i++) > > Is a little more idiomatic Sizing of the array using some macro / enum that accounts for u32 walking versus u8 walking means the sizeof goes away. > >> - data_cmp = cm_compare_private_data(private_data, >> + data_cmp = cm_compare_private_data((u32 *) private_data, >> cm_id_priv->compare_data); > > I'd be happier without this cast, at the worst move it up one more > layer, at the best, fix the two impacted structures as well. I tried to fix private_data and it blows the patch up. > >> I feel like I stepped into a tar pit... It's a bit odd that >> IB_CM_COMPARE_SIZE is part of an enum rather than a #define. > > This is, again, idiomatic. '#define CONSTANT' is often frowned upon, > the semantics of enum are much cleaner/safer, '#define NAME VALUE' is used throughout the kernel. Why does ib_rdma need to be different? >>> Anyway, you might want to compile this part of the kernel with >>> -Wcast-align and ignore the false positive (in particular >>> every other use of container_of()). > >> Why? > > To look for more sketchy things that might impact SPARC, ARM, etc. Seems like a topic for another patch. David -- 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/core/cm.c b/drivers/infiniband/core/cm.c index e28a494e2a3a..cbbf741987b3 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -437,20 +437,19 @@ static struct cm_id_private * cm_acquire_id(__be32 local_id, __be32 remote_id) return cm_id_priv; } -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask) { int i; - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & - ((unsigned long *) mask)[i]; + for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++) + dst[i] = src[i] & mask[i]; } static int cm_compare_data(struct ib_cm_compare_data *src_data, struct ib_cm_compare_data *dst_data) { - u8 src[IB_CM_COMPARE_SIZE]; - u8 dst[IB_CM_COMPARE_SIZE]; + u32 src[IB_CM_COMPARE_SIZE]; + u32 dst[IB_CM_COMPARE_SIZE]; if (!src_data || !dst_data) return 0; @@ -460,10 +459,10 @@ static int cm_compare_data(struct ib_cm_compare_data *src_data, return memcmp(src, dst, IB_CM_COMPARE_SIZE); } -static int cm_compare_private_data(u8 *private_data, +static int cm_compare_private_data(u32 *private_data, struct ib_cm_compare_data *dst_data) { - u8 src[IB_CM_COMPARE_SIZE]; + u32 src[IB_CM_COMPARE_SIZE]; if (!dst_data) return 0; @@ -546,7 +545,7 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device, while (node) { cm_id_priv = rb_entry(node, struct cm_id_private, service_node); - data_cmp = cm_compare_private_data(private_data, + data_cmp = cm_compare_private_data((u32 *) private_data, cm_id_priv->compare_data); if ((cm_id_priv->id.service_mask & service_id) == cm_id_priv->id.service_id && diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h index 0e3ff30647d5..0dd576f40c83 100644 --- a/include/rdma/ib_cm.h +++ b/include/rdma/ib_cm.h @@ -337,8 +337,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id); #define IB_SDP_SERVICE_ID_MASK cpu_to_be64(0xFFFFFFFFFFFF0000ULL) struct ib_cm_compare_data { - u8 data[IB_CM_COMPARE_SIZE]; - u8 mask[IB_CM_COMPARE_SIZE]; + u32 data[IB_CM_COMPARE_SIZE]; + u32 mask[IB_CM_COMPARE_SIZE]; }; /**
Addresses the following kernel logs seen during boot of sparc systems: Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason Gunthorpe. Signed-off-by: David Ahern <david.ahern@oracle.com> --- v2 - updated per Jason's comments drivers/infiniband/core/cm.c | 17 ++++++++--------- include/rdma/ib_cm.h | 4 ++-- 2 files changed, 10 insertions(+), 11 deletions(-)