* Re: Seeing new kernel unaligned access messages in linux-next on ia64 [not found] ` <20100324.102759.107122703.davem@davemloft.net> @ 2010-03-24 21:26 ` Jan Engelhardt 2010-03-24 22:47 ` Andreas Schwab 0 siblings, 1 reply; 10+ messages in thread From: Jan Engelhardt @ 2010-03-24 21:26 UTC (permalink / raw) To: David Miller; +Cc: tony.luck, netdev Hi, Tony Luck observes that the original IFLA_STATS64 submission causes unaligned accesses. This is because nla_data() returns a pointer to a memory region that is only aligned to 32 bits. Using a temporary and memcpying it off would normally fix this, as in the patch below. During testing however, I still get unaligned messages even with the patch - and I would not know what causes this. In fact, adding a printks magically fixes it. (Bug in gcc-4.4-sparc compiler?) memcpy(v, &a, sizeof(a)); + printk(KERN_INFO "v=%p a=%p\n", v, &a); origin git://dev.medozas.de/linux net mode cherry-pick parent 1c01fe14a87332cc88266fbd6e598319322eb96f (v2.6.34-rc1-1069-g1c01fe1) commit 5480c9bb1b418bb09748340257dea1e57efeb18f Author: Jan Engelhardt <jengelh@medozas.de> Date: Wed Mar 24 19:52:43 2010 +0100 net: fix unaligned access in IFLA_STATS64 Tony Luck observes that the original IFLA_STATS64 submission causes unaligned accesses. This is because nla_data() returns a pointer to a memory region that is only aligned to 32 bits. Do some memcpying to workaround this. Signed-off-by: Jan Engelhardt <jengelh@medozas.de> --- net/core/rtnetlink.c | 53 +++++++++++++++++++++-------------------- 1 files changed, 27 insertions(+), 26 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index e1121f0..473d4b1 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -602,36 +602,39 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a, a->tx_compressed = b->tx_compressed; } -static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *a, +static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *v, const struct net_device_stats *b) { - a->rx_packets = b->rx_packets; - a->tx_packets = b->tx_packets; - a->rx_bytes = b->rx_bytes; - a->tx_bytes = b->tx_bytes; - a->rx_errors = b->rx_errors; - a->tx_errors = b->tx_errors; - a->rx_dropped = b->rx_dropped; - a->tx_dropped = b->tx_dropped; - - a->multicast = b->multicast; - a->collisions = b->collisions; - - a->rx_length_errors = b->rx_length_errors; - a->rx_over_errors = b->rx_over_errors; - a->rx_crc_errors = b->rx_crc_errors; - a->rx_frame_errors = b->rx_frame_errors; - a->rx_fifo_errors = b->rx_fifo_errors; - a->rx_missed_errors = b->rx_missed_errors; - - a->tx_aborted_errors = b->tx_aborted_errors; - a->tx_carrier_errors = b->tx_carrier_errors; - a->tx_fifo_errors = b->tx_fifo_errors; - a->tx_heartbeat_errors = b->tx_heartbeat_errors; - a->tx_window_errors = b->tx_window_errors; - - a->rx_compressed = b->rx_compressed; - a->tx_compressed = b->tx_compressed; + struct rtnl_link_stats64 a; + + a.rx_packets = b->rx_packets; + a.tx_packets = b->tx_packets; + a.rx_bytes = b->rx_bytes; + a.tx_bytes = b->tx_bytes; + a.rx_errors = b->rx_errors; + a.tx_errors = b->tx_errors; + a.rx_dropped = b->rx_dropped; + a.tx_dropped = b->tx_dropped; + + a.multicast = b->multicast; + a.collisions = b->collisions; + + a.rx_length_errors = b->rx_length_errors; + a.rx_over_errors = b->rx_over_errors; + a.rx_crc_errors = b->rx_crc_errors; + a.rx_frame_errors = b->rx_frame_errors; + a.rx_fifo_errors = b->rx_fifo_errors; + a.rx_missed_errors = b->rx_missed_errors; + + a.tx_aborted_errors = b->tx_aborted_errors; + a.tx_carrier_errors = b->tx_carrier_errors; + a.tx_fifo_errors = b->tx_fifo_errors; + a.tx_heartbeat_errors = b->tx_heartbeat_errors; + a.tx_window_errors = b->tx_window_errors; + + a.rx_compressed = b->rx_compressed; + a.tx_compressed = b->tx_compressed; + memcpy(v, &a, sizeof(a)); } static inline int rtnl_vfinfo_size(const struct net_device *dev) @@ -734,8 +737,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, sizeof(struct rtnl_link_stats64)); if (attr == NULL) goto nla_put_failure; - - stats = dev_get_stats(dev); copy_rtnl_link_stats64(nla_data(attr), stats); if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) { -- # Created with git-export-patch ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-24 21:26 ` Seeing new kernel unaligned access messages in linux-next on ia64 Jan Engelhardt @ 2010-03-24 22:47 ` Andreas Schwab 2010-03-24 23:17 ` Jan Engelhardt 0 siblings, 1 reply; 10+ messages in thread From: Andreas Schwab @ 2010-03-24 22:47 UTC (permalink / raw) To: Jan Engelhardt; +Cc: David Miller, tony.luck, netdev Jan Engelhardt <jengelh@medozas.de> writes: > Using a temporary and memcpying it off would normally fix this, > as in the patch below. During testing however, I still get > unaligned messages even with the patch - and I would not know > what causes this. The memcpy will not fix the alignment issue because the copy operation is fully equivalent to a direct assignment, and the compiler can still take advantage of the known alignment of the types. You have to explicitly tell the compiler about the reduced alignment guarantee. > In fact, adding a printks magically fixes it. (Bug in gcc-4.4-sparc > compiler?) > > memcpy(v, &a, sizeof(a)); > + printk(KERN_INFO "v=%p a=%p\n", v, &a); Presumably the extended lifetime of the variables caused the compiler to use a different expansion for memcpy which is less dependent on alignment. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-24 22:47 ` Andreas Schwab @ 2010-03-24 23:17 ` Jan Engelhardt 2010-03-24 23:19 ` Luck, Tony 2010-03-25 3:32 ` David Miller 0 siblings, 2 replies; 10+ messages in thread From: Jan Engelhardt @ 2010-03-24 23:17 UTC (permalink / raw) To: Andreas Schwab; +Cc: David Miller, tony.luck, netdev On Wednesday 2010-03-24 23:47, Andreas Schwab wrote: >Jan Engelhardt writes: > >> Using a temporary and memcpying it off would normally fix this, >> as in the patch below. During testing however, I still get >> unaligned messages even with the patch - and I would not know >> what causes this. > >The memcpy will not fix the alignment issue because the copy operation >is fully equivalent to a direct assignment, and the compiler can still >take advantage of the known alignment of the types. You have to >explicitly tell the compiler about the reduced alignment guarantee. You're right, I remember seeing that sort of optimization before. So I have changed the function's signature to read -static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *v, - const struct net_device_stats *b) +static void copy_rtnl_link_stats64(void *v, const struct net_device_stats *b) { ... memcpy(v, &a, sizeof(a)); } No more unaligned messages - but is this an acceptable solution? thanks, Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-24 23:17 ` Jan Engelhardt @ 2010-03-24 23:19 ` Luck, Tony 2010-03-25 3:32 ` David Miller 2010-03-25 3:32 ` David Miller 1 sibling, 1 reply; 10+ messages in thread From: Luck, Tony @ 2010-03-24 23:19 UTC (permalink / raw) To: Jan Engelhardt, Andreas Schwab; +Cc: David Miller, netdev@vger.kernel.org > No more unaligned messages - but is this an acceptable solution? There are a bunch of macros in include/linux/unaligned/*.h to handle this sort of thing. -Tony ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-24 23:19 ` Luck, Tony @ 2010-03-25 3:32 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2010-03-25 3:32 UTC (permalink / raw) To: tony.luck; +Cc: jengelh, schwab, netdev From: "Luck, Tony" <tony.luck@intel.com> Date: Wed, 24 Mar 2010 16:19:38 -0700 >> No more unaligned messages - but is this an acceptable solution? > > There are a bunch of macros in include/linux/unaligned/*.h to > handle this sort of thing. It's totally unnecessary here and it would be overkill to use those interfaces one at a time on every struct member when a proper memcpy() fro ma type-pruned void pointer suffices. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-24 23:17 ` Jan Engelhardt 2010-03-24 23:19 ` Luck, Tony @ 2010-03-25 3:32 ` David Miller 2010-03-27 12:14 ` Jan Engelhardt 1 sibling, 1 reply; 10+ messages in thread From: David Miller @ 2010-03-25 3:32 UTC (permalink / raw) To: jengelh; +Cc: schwab, tony.luck, netdev From: Jan Engelhardt <jengelh@medozas.de> Date: Thu, 25 Mar 2010 00:17:01 +0100 (CET) > No more unaligned messages - but is this an acceptable solution? We already rely on this elsewhere, particularly in the xfrm_user code. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-25 3:32 ` David Miller @ 2010-03-27 12:14 ` Jan Engelhardt 2010-03-27 23:37 ` David Miller 0 siblings, 1 reply; 10+ messages in thread From: Jan Engelhardt @ 2010-03-27 12:14 UTC (permalink / raw) To: David Miller; +Cc: schwab, tony.luck, netdev On Thursday 2010-03-25 04:32, David Miller wrote: >From: Jan Engelhardt >Date: Thu, 25 Mar 2010 00:17:01 +0100 (CET) > >> No more unaligned messages - but is this [using void*] >> an acceptable solution? > >We already rely on this elsewhere, particularly in the >xfrm_user code. Ok, here's it. (We're also getting rid of the double-obtain statistics that was mentioned in the initial submission.) Please apply this one, or pull, or cherry-pick from git://dev.medozas.de/linux net parent b79d1d54cf0672f764402fe4711ef5306f917bd3 (v2.6.34-rc1-1275-gb79d1d5) commit c5c57d7c7837858aa499610a3ee760b39f1de937 Author: Jan Engelhardt <jengelh@medozas.de> Date: Wed Mar 24 19:52:43 2010 +0100 net: fix unaligned access in IFLA_STATS64 Tony Luck observes that the original IFLA_STATS64 submission causes unaligned accesses. This is because nla_data() returns a pointer to a memory region that is only aligned to 32 bits. Do some memcpying to workaround this. Signed-off-by: Jan Engelhardt <jengelh@medozas.de> --- net/core/rtnetlink.c | 54 +++++++++++++++++++++--------------------- 1 files changed, 27 insertions(+), 27 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index ffc6cf3..ed0766f 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -602,36 +602,38 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a, a->tx_compressed = b->tx_compressed; } -static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *a, - const struct net_device_stats *b) +static void copy_rtnl_link_stats64(void *v, const struct net_device_stats *b) { - a->rx_packets = b->rx_packets; - a->tx_packets = b->tx_packets; - a->rx_bytes = b->rx_bytes; - a->tx_bytes = b->tx_bytes; - a->rx_errors = b->rx_errors; - a->tx_errors = b->tx_errors; - a->rx_dropped = b->rx_dropped; - a->tx_dropped = b->tx_dropped; - - a->multicast = b->multicast; - a->collisions = b->collisions; - - a->rx_length_errors = b->rx_length_errors; - a->rx_over_errors = b->rx_over_errors; - a->rx_crc_errors = b->rx_crc_errors; - a->rx_frame_errors = b->rx_frame_errors; - a->rx_fifo_errors = b->rx_fifo_errors; - a->rx_missed_errors = b->rx_missed_errors; - - a->tx_aborted_errors = b->tx_aborted_errors; - a->tx_carrier_errors = b->tx_carrier_errors; - a->tx_fifo_errors = b->tx_fifo_errors; - a->tx_heartbeat_errors = b->tx_heartbeat_errors; - a->tx_window_errors = b->tx_window_errors; - - a->rx_compressed = b->rx_compressed; - a->tx_compressed = b->tx_compressed; + struct rtnl_link_stats64 a; + + a.rx_packets = b->rx_packets; + a.tx_packets = b->tx_packets; + a.rx_bytes = b->rx_bytes; + a.tx_bytes = b->tx_bytes; + a.rx_errors = b->rx_errors; + a.tx_errors = b->tx_errors; + a.rx_dropped = b->rx_dropped; + a.tx_dropped = b->tx_dropped; + + a.multicast = b->multicast; + a.collisions = b->collisions; + + a.rx_length_errors = b->rx_length_errors; + a.rx_over_errors = b->rx_over_errors; + a.rx_crc_errors = b->rx_crc_errors; + a.rx_frame_errors = b->rx_frame_errors; + a.rx_fifo_errors = b->rx_fifo_errors; + a.rx_missed_errors = b->rx_missed_errors; + + a.tx_aborted_errors = b->tx_aborted_errors; + a.tx_carrier_errors = b->tx_carrier_errors; + a.tx_fifo_errors = b->tx_fifo_errors; + a.tx_heartbeat_errors = b->tx_heartbeat_errors; + a.tx_window_errors = b->tx_window_errors; + + a.rx_compressed = b->rx_compressed; + a.tx_compressed = b->tx_compressed; + memcpy(v, &a, sizeof(a)); } static inline int rtnl_vfinfo_size(const struct net_device *dev) @@ -734,8 +736,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, sizeof(struct rtnl_link_stats64)); if (attr == NULL) goto nla_put_failure; - - stats = dev_get_stats(dev); copy_rtnl_link_stats64(nla_data(attr), stats); if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) { -- # Created with git-export-patch ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-27 12:14 ` Jan Engelhardt @ 2010-03-27 23:37 ` David Miller 2010-03-28 0:11 ` Jan Engelhardt 0 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2010-03-27 23:37 UTC (permalink / raw) To: jengelh; +Cc: schwab, tony.luck, netdev From: Jan Engelhardt <jengelh@medozas.de> Date: Sat, 27 Mar 2010 13:14:46 +0100 (CET) > net: fix unaligned access in IFLA_STATS64 Applied to net-next-2.6, thanks Jan. Hey, don't we need some adjustments to if_nlmsg_size()? I don't see it accounting for IFLA_STATS64/"struct rtnl_link_stats64" there. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-27 23:37 ` David Miller @ 2010-03-28 0:11 ` Jan Engelhardt 2010-03-28 0:17 ` David Miller 0 siblings, 1 reply; 10+ messages in thread From: Jan Engelhardt @ 2010-03-28 0:11 UTC (permalink / raw) To: David Miller; +Cc: schwab, tony.luck, netdev On Sunday 2010-03-28 00:37, David Miller wrote: > >> net: fix unaligned access in IFLA_STATS64 > >Applied to net-next-2.6, thanks Jan. > >Hey, don't we need some adjustments to if_nlmsg_size()? I don't see >it accounting for IFLA_STATS64/"struct rtnl_link_stats64" there. If I am not mistaken, the answer is "not strictly". But of course it's nicer if we don't need to realloc just because we were too conservative in the initial calculation. git://dev.medozas.de/linux net parent c5c57d7c7837858aa499610a3ee760b39f1de937 (v2.6.34-rc1-1276-gc5c57d7) commit 305876b7c1c720db30239d08d56b3a058d56aa21 Author: Jan Engelhardt <jengelh@medozas.de> Date: Sun Mar 28 01:03:32 2010 +0100 net: increase preallocated size of nlmsg to accomodate for IFLA_STATS64 When more data is stuffed into an nlmsg than initially projected, an extra allocation needs to be done. Reserve enough for IFLA_STATS64 so that this does not to needlessy happen. Signed-off-by: Jan Engelhardt <jengelh@medozas.de> --- net/core/rtnetlink.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index ed0766f..bf919b6 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -653,6 +653,7 @@ static inline size_t if_nlmsg_size(const struct net_device *dev) + nla_total_size(IFNAMSIZ) /* IFLA_QDISC */ + nla_total_size(sizeof(struct rtnl_link_ifmap)) + nla_total_size(sizeof(struct rtnl_link_stats)) + + nla_total_size(sizeof(struct rtnl_link_stats64)) + nla_total_size(MAX_ADDR_LEN) /* IFLA_ADDRESS */ + nla_total_size(MAX_ADDR_LEN) /* IFLA_BROADCAST */ + nla_total_size(4) /* IFLA_TXQLEN */ -- # Created with git-export-patch ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Seeing new kernel unaligned access messages in linux-next on ia64 2010-03-28 0:11 ` Jan Engelhardt @ 2010-03-28 0:17 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2010-03-28 0:17 UTC (permalink / raw) To: jengelh; +Cc: schwab, tony.luck, netdev From: Jan Engelhardt <jengelh@medozas.de> Date: Sun, 28 Mar 2010 01:11:13 +0100 (CET) > On Sunday 2010-03-28 00:37, David Miller wrote: >> >>> net: fix unaligned access in IFLA_STATS64 >> >>Applied to net-next-2.6, thanks Jan. >> >>Hey, don't we need some adjustments to if_nlmsg_size()? I don't see >>it accounting for IFLA_STATS64/"struct rtnl_link_stats64" there. > > If I am not mistaken, the answer is "not strictly". But of course it's > nicer if we don't need to realloc just because we were too conservative > in the initial calculation. Right. > net: increase preallocated size of nlmsg to accomodate for IFLA_STATS64 > > When more data is stuffed into an nlmsg than initially projected, an > extra allocation needs to be done. Reserve enough for IFLA_STATS64 so > that this does not to needlessy happen. > > Signed-off-by: Jan Engelhardt <jengelh@medozas.de> Applied, thanks a lot! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-03-28 0:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <12c511ca1003181112t6098eeafm30e384e755a42185@mail.gmail.com>
[not found] ` <alpine.LSU.2.01.1003192310450.10055@obet.zrqbmnf.qr>
[not found] ` <20100324.102759.107122703.davem@davemloft.net>
2010-03-24 21:26 ` Seeing new kernel unaligned access messages in linux-next on ia64 Jan Engelhardt
2010-03-24 22:47 ` Andreas Schwab
2010-03-24 23:17 ` Jan Engelhardt
2010-03-24 23:19 ` Luck, Tony
2010-03-25 3:32 ` David Miller
2010-03-25 3:32 ` David Miller
2010-03-27 12:14 ` Jan Engelhardt
2010-03-27 23:37 ` David Miller
2010-03-28 0:11 ` Jan Engelhardt
2010-03-28 0:17 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).