| Message ID | alpine.LSU.2.01.1003242059230.4028@obet.zrqbmnf.qr |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/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) {
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(-)