[dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Jan Viktorin]

Hello,
	
I was looking at some warnings generated during ARM build. I can see
53 warnings for my build based on v2.2.0-rc3, spread among:

 app/test-pmd/{flowgen,icmpecho,txonly}.c
 app/test/{packet_burst_generator,test_hash_functions,test_thash}.c
 lib/librte_ether/rte_ether.h
 drivers/net/bonding/rte_eth_bond_pmd.c
 lib/librte_acl/{acl_gen,acl_run}.c
 lib/librte_eal/linuxapp/eal/{eal_interrupts,eal_pci_vfio_mp_sync}.c
 lib/librte_hash/rte_cuckoo_hash.c
 lib/librte_ip_frag/rte_ipv4_reassembly.c
 lib/librte_sched/{rte_bitmap.h,rte_sched.c}

I think, some of them are false-positives. In this RFC patch I tried to fix
only the rte_ether.h which uses the unaligned_uint16_t data type. I didn't
test it as it is just the first kick to solve more of those warns.

Regards
Jan

(I considered to not add the cover-letter as this is just a single small patch.
I hope it does not matter a lot. Is there any convention how to do this?)
---
This commit removes warning reported when building for ARMv7 target.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
---
 lib/librte_ether/rte_ether.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ether/rte_ether.h b/lib/librte_ether/rte_ether.h
index 07c17d7..ba8a80a 100644
--- a/lib/librte_ether/rte_ether.h
+++ b/lib/librte_ether/rte_ether.h
@@ -175,10 +175,9 @@ static inline int is_multicast_ether_addr(const struct ether_addr *ea)
  */
 static inline int is_broadcast_ether_addr(const struct ether_addr *ea)
 {
-	const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
+	const uint32_t *ea_words = (const uint32_t *)ea;
 
-	return (ea_words[0] == 0xFFFF && ea_words[1] == 0xFFFF &&
-		ea_words[2] == 0xFFFF);
+	return ea_words[0] == 0xFFFFFFFF && (ea_words[1] & 0x0FFFF) == 0x0FFFF;
 }
 
 /**
-- 
2.6.3

Re: [dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Stephen Hemminger]

On Tue,  8 Dec 2015 20:29:53 +0100
Jan Viktorin <viktorin@rehivetech.com> wrote:

> Hello,
> 	
> I was looking at some warnings generated during ARM build. I can see
> 53 warnings for my build based on v2.2.0-rc3, spread among:
> 
>  app/test-pmd/{flowgen,icmpecho,txonly}.c
>  app/test/{packet_burst_generator,test_hash_functions,test_thash}.c
>  lib/librte_ether/rte_ether.h
>  drivers/net/bonding/rte_eth_bond_pmd.c
>  lib/librte_acl/{acl_gen,acl_run}.c
>  lib/librte_eal/linuxapp/eal/{eal_interrupts,eal_pci_vfio_mp_sync}.c
>  lib/librte_hash/rte_cuckoo_hash.c
>  lib/librte_ip_frag/rte_ipv4_reassembly.c
>  lib/librte_sched/{rte_bitmap.h,rte_sched.c}
> 
> I think, some of them are false-positives. In this RFC patch I tried to fix
> only the rte_ether.h which uses the unaligned_uint16_t data type. I didn't
> test it as it is just the first kick to solve more of those warns.
> 
> Regards
> Jan
> 
> (I considered to not add the cover-letter as this is just a single small patch.
> I hope it does not matter a lot. Is there any convention how to do this?)
> ---
> This commit removes warning reported when building for ARMv7 target.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
>  lib/librte_ether/rte_ether.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ether.h b/lib/librte_ether/rte_ether.h
> index 07c17d7..ba8a80a 100644
> --- a/lib/librte_ether/rte_ether.h
> +++ b/lib/librte_ether/rte_ether.h
> @@ -175,10 +175,9 @@ static inline int is_multicast_ether_addr(const struct ether_addr *ea)
>   */
>  static inline int is_broadcast_ether_addr(const struct ether_addr *ea)
>  {
> -	const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
> +	const uint32_t *ea_words = (const uint32_t *)ea;
>  
> -	return (ea_words[0] == 0xFFFF && ea_words[1] == 0xFFFF &&
> -		ea_words[2] == 0xFFFF);
> +	return ea_words[0] == 0xFFFFFFFF && (ea_words[1] & 0x0FFFF) == 0x0FFFF;

The problem with that is that it assumes little-endian.

Re: [dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Thomas Monjalon]

2015-12-08 20:29, Jan Viktorin:
> (I considered to not add the cover-letter as this is just a single small patch.
> I hope it does not matter a lot. Is there any convention how to do this?)

The main interest of splitting patches or adding a cover letter it to have
a place to explain the changes. When you have several changes, they deserve
several patches to provide an accurate explanation. In such case, you may
need a cover letter to describe the global idea of the series. A cover letter
is also helpful for mail threading and acking all the series.
When you have only one change, one email is enough.

John, should we add this explanation in the contributing guide?

Re: [dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Jan Viktorin]

On Tue, 8 Dec 2015 12:02:54 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Tue,  8 Dec 2015 20:29:53 +0100
> Jan Viktorin <viktorin@rehivetech.com> wrote:
> 
> > Hello,
> > 	
> > I was looking at some warnings generated during ARM build. I can see
> > 53 warnings for my build based on v2.2.0-rc3, spread among:
> > 
> >  app/test-pmd/{flowgen,icmpecho,txonly}.c
> >  app/test/{packet_burst_generator,test_hash_functions,test_thash}.c
> >  lib/librte_ether/rte_ether.h
> >  drivers/net/bonding/rte_eth_bond_pmd.c
> >  lib/librte_acl/{acl_gen,acl_run}.c
> >  lib/librte_eal/linuxapp/eal/{eal_interrupts,eal_pci_vfio_mp_sync}.c
> >  lib/librte_hash/rte_cuckoo_hash.c
> >  lib/librte_ip_frag/rte_ipv4_reassembly.c
> >  lib/librte_sched/{rte_bitmap.h,rte_sched.c}
> > 
> > I think, some of them are false-positives. In this RFC patch I tried to fix
> > only the rte_ether.h which uses the unaligned_uint16_t data type. I didn't
> > test it as it is just the first kick to solve more of those warns.
> > 
> > Regards
> > Jan
> > 
> > (I considered to not add the cover-letter as this is just a single small patch.
> > I hope it does not matter a lot. Is there any convention how to do this?)
> > ---
> > This commit removes warning reported when building for ARMv7 target.
> > 
> > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> > ---
> >  lib/librte_ether/rte_ether.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/librte_ether/rte_ether.h b/lib/librte_ether/rte_ether.h
> > index 07c17d7..ba8a80a 100644
> > --- a/lib/librte_ether/rte_ether.h
> > +++ b/lib/librte_ether/rte_ether.h
> > @@ -175,10 +175,9 @@ static inline int is_multicast_ether_addr(const struct ether_addr *ea)
> >   */
> >  static inline int is_broadcast_ether_addr(const struct ether_addr *ea)
> >  {
> > -	const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
> > +	const uint32_t *ea_words = (const uint32_t *)ea;
> >  
> > -	return (ea_words[0] == 0xFFFF && ea_words[1] == 0xFFFF &&
> > -		ea_words[2] == 0xFFFF);
> > +	return ea_words[0] == 0xFFFFFFFF && (ea_words[1] & 0x0FFFF) == 0x0FFFF;  
> 
> The problem with that is that it assumes little-endian.

Well, true. But there is another issue that I considered just after
left for home today :). The ether_addr can be unaligned in memory easily
(as it comes in the Ethernet frame). Another false-positive... So the
original code is correct. The fix would be to mute the compiler here.

The solution is probably to enable RTE_ARCH_STRICT_ALIGN in the
defconfig_arm-armv7a-linuxapp-gcc. This will probably solve more unaligned false-positives.

Regards
Jan

-- 
  Jan Viktorin                E-mail: Viktorin@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech
  Brno, Czech Republic

Re: [dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Jan Viktorin]

On Tue, 08 Dec 2015 21:30:03 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-12-08 20:29, Jan Viktorin:
> > (I considered to not add the cover-letter as this is just a single small patch.
> > I hope it does not matter a lot. Is there any convention how to do this?)  
> 
> The main interest of splitting patches or adding a cover letter it to have
> a place to explain the changes. When you have several changes, they deserve
> several patches to provide an accurate explanation. In such case, you may
> need a cover letter to describe the global idea of the series. A cover letter
> is also helpful for mail threading and acking all the series.
> When you have only one change, one email is enough.

That's good. Should there be a way how to separate the "inline-cover"
and the commit?

> 
> John, should we add this explanation in the contributing guide?



-- 
  Jan Viktorin                E-mail: Viktorin@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech
  Brno, Czech Republic

Re: [dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Thomas Monjalon]

2015-12-08 21:55, Jan Viktorin:
> On Tue, 08 Dec 2015 21:30:03 +0100
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> > 2015-12-08 20:29, Jan Viktorin:
> > > (I considered to not add the cover-letter as this is just a single small patch.
> > > I hope it does not matter a lot. Is there any convention how to do this?)  
> > 
> > The main interest of splitting patches or adding a cover letter it to have
> > a place to explain the changes. When you have several changes, they deserve
> > several patches to provide an accurate explanation. In such case, you may
> > need a cover letter to describe the global idea of the series. A cover letter
> > is also helpful for mail threading and acking all the series.
> > When you have only one change, one email is enough.
> 
> That's good. Should there be a way how to separate the "inline-cover"
> and the commit?

Do you mean how add a comment which won't be in the git tree?
You can put whatever between the 3 dashes and the diff.

Re: [dpdk-dev] [[RFC PATCH]] lib/ether: fix 16-bit unaligned access

[Jan Viktorin]

On Tue, 08 Dec 2015 21:57:33 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-12-08 21:55, Jan Viktorin:
> > On Tue, 08 Dec 2015 21:30:03 +0100
> > Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> >   
> > > 2015-12-08 20:29, Jan Viktorin:  
> > > > (I considered to not add the cover-letter as this is just a single small patch.
> > > > I hope it does not matter a lot. Is there any convention how to do this?)    
> > > 
> > > The main interest of splitting patches or adding a cover letter it to have
> > > a place to explain the changes. When you have several changes, they deserve
> > > several patches to provide an accurate explanation. In such case, you may
> > > need a cover letter to describe the global idea of the series. A cover letter
> > > is also helpful for mail threading and acking all the series.
> > > When you have only one change, one email is enough.  
> > 
> > That's good. Should there be a way how to separate the "inline-cover"
> > and the commit?  
> 
> Do you mean how add a comment which won't be in the git tree?
> You can put whatever between the 3 dashes and the diff.
> 

Exactly. When I was writing this message, I decided to put the "cover"
first and the commit log after the 3 dashes. It was more logical to
me...

[dpdk-dev] [PATCH 1/1] arm: set CONFIG_RTE_ARCH_STRICT_ALIGN=y for armv7 target

[Jan Viktorin]

This patch reduces number of warnings from 53 to 40. It removes the usual false
positives utilizing unaligned_uint*_t data types.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
---
As far as I know, only a 64-bit unaligned access can be a problem for ARMv7.
I found only one such occurence:

118 struct rte_mbuf *                                                               
119 rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,                    
120                 struct rte_ip_frag_death_row *dr, struct rte_mbuf *mb, uint64_t tms,
121                 struct ipv4_hdr *ip_hdr)
...
133         psd = (unaligned_uint64_t *)&ip_hdr->src_addr;                          
134         /* use first 8 bytes only */                                            
135         key.src_dst[0] = psd[0];                                                
136         key.id = ip_hdr->packet_id;                                             
137         key.key_len = IPV4_KEYLEN

Is this a real issue?
---
 config/defconfig_arm-armv7a-linuxapp-gcc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config/defconfig_arm-armv7a-linuxapp-gcc b/config/defconfig_arm-armv7a-linuxapp-gcc
index cbebd64..2482d91 100644
--- a/config/defconfig_arm-armv7a-linuxapp-gcc
+++ b/config/defconfig_arm-armv7a-linuxapp-gcc
@@ -39,6 +39,7 @@ CONFIG_RTE_ARCH_ARM_TUNE="cortex-a9"
 CONFIG_RTE_ARCH_ARM_NEON=y
 
 CONFIG_RTE_FORCE_INTRINSICS=y
+CONFIG_RTE_ARCH_STRICT_ALIGN=y
 
 CONFIG_RTE_TOOLCHAIN="gcc"
 CONFIG_RTE_TOOLCHAIN_GCC=y
-- 
2.6.3

Re: [dpdk-dev] [PATCH 1/1] arm: set CONFIG_RTE_ARCH_STRICT_ALIGN=y for armv7 target

[Thomas Monjalon]

2015-12-09 16:16, Jan Viktorin:
> This patch reduces number of warnings from 53 to 40. It removes the usual false
> positives utilizing unaligned_uint*_t data types.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>

Applied, thanks

Jan, what is the problem with the other ARM alignment warnings?
Can they be fixed?

Re: [dpdk-dev] [PATCH 1/1] arm: set CONFIG_RTE_ARCH_STRICT_ALIGN=y for armv7 target

[Jan Viktorin]

On Mon, 29 Feb 2016 16:14:58 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-12-09 16:16, Jan Viktorin:
> > This patch reduces number of warnings from 53 to 40. It removes the usual false
> > positives utilizing unaligned_uint*_t data types.
> > 
> > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>  
> 
> Applied, thanks
> 
> Jan, what is the problem with the other ARM alignment warnings?
> Can they be fixed?

This is the full list of warnings I can see on the current origin/master
for ARMv7 (42 occurences) including examples (+10 more). The origin of
all of them is:

  cast increases required alignment of target type [-Wcast-align]

After skimming through the list, you can see that they are mostly casts
to uint32_t * or something similar. I believe that all of them are OK.
However, I don't know how to persuade GCC to not be angry...

Probably, we can add some explicit alignment of certain structures.

app/test/test_thash.c
116   rte_convert_rss_key((uint32_t *)&default_rss_key,
117     (uint32_t *)rss_key_be, RTE_DIM(default_rss_key));

build/include/test_thash.h
179     *((uint32_t *)targ->v6.src_addr + i) =
180       rte_be_to_cpu_32(*((const uint32_t *)orig->src_addr + i));
181     *((uint32_t *)targ->v6.dst_addr + i) =
182       rte_be_to_cpu_32(*((const uint32_t *)orig->dst_addr + i));
207         ret ^= rte_cpu_to_be_32(((const uint32_t *)rss_key)[j]) << i |
208           (uint32_t)((uint64_t)(rte_cpu_to_be_32(((const uint32_t *)rss_key)[j + 1])) >>
238         ret ^= ((const uint32_t *)rss_key)[j] << i |
239           (uint32_t)((uint64_t)(((const uint32_t *)rss_key)[j + 1]) >> (32 - i));

examples-sdk/usr/local/share/dpdk/arm-armv7a-linuxapp-gcc/include/rte_mbuf.h
1617   ((t)((char *)(m)->buf_addr + (m)->data_off + (o)))

examples/l3fwd-acl/main.c
1074       next = (struct rte_acl_rule *)(route_rules +
1079       next = (struct rte_acl_rule *)(acl_rules +
1115   *pacl_base = (struct rte_acl_rule *)acl_rules;
1117   *proute_base = (struct rte_acl_rule *)route_rules;

netmap_user.h
65 #define NETMAP_IF(b, o)  (struct netmap_if *)((char *)(b) + (o))
68   ((struct netmap_ring *)((char *)(nifp) +  \
72   ((struct netmap_ring *)((char *)(nifp) +  \

examples/vhost/main.c
121 #define MBUF_HEADROOM_UINT32(mbuf) (*(uint32_t *)((uint8_t *)(mbuf) \
945   return ((*(uint64_t *)ea ^ *(uint64_t *)eb) & MAC_ADDR_CMP) == 0;

lib/librte_acl/acl_gen.c
391     qtrp = (uint32_t *)node->transitions;

lib/librte_acl/acl_run.h
46   (*((const int32_t *)((prm)[(idx)].data + *(prm)[idx].data_index++)))

lib/librte_eal/linuxapp/eal/eal_interrupts.c
150   irq_set = (struct vfio_irq_set *) irq_set_buf;
156   fd_ptr = (int *) &irq_set->data;
196   irq_set = (struct vfio_irq_set *) irq_set_buf;
239   irq_set = (struct vfio_irq_set *) irq_set_buf;
245   fd_ptr = (int *) &irq_set->data;
267   irq_set = (struct vfio_irq_set *) irq_set_buf;
293   irq_set = (struct vfio_irq_set *) irq_set_buf;
304   fd_ptr = (int *) &irq_set->data;
330   irq_set = (struct vfio_irq_set *) irq_set_buf;

lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
176   chdr = (struct cmsghdr *) chdr_buf;
209   chdr = (struct cmsghdr *) chdr_buf;

595       k = (struct rte_hash_key *) ((char *)keys +
615       k = (struct rte_hash_key *) ((char *)keys +
726       k = (struct rte_hash_key *) ((char *)keys +
749       k = (struct rte_hash_key *) ((char *)keys +
841       k = (struct rte_hash_key *) ((char *)keys +
864       k = (struct rte_hash_key *) ((char *)keys +
959   *key_slot = (const struct rte_hash_key *) ((const char *)keys +
1233   next_key = (struct rte_hash_key *) ((char *)h->key_store +

lib/librte_sched/rte_bitmap.h
262   bmp = (struct rte_bitmap *) mem;
264   bmp->array1 = (uint64_t *) &mem[array1_byte_offset];
266   bmp->array2 = (uint64_t *) &mem[array2_byte_offset];

lib/librte_sched/rte_sched.c
684   port->subport = (struct rte_sched_subport *)
687   port->pipe = (struct rte_sched_pipe *)
690   port->queue = (struct rte_sched_queue *)
693   port->queue_extra = (struct rte_sched_queue_extra *)
696   port->pipe_profiles = (struct rte_sched_pipe_profile *)
701   port->queue_array = (struct rte_mbuf **)

lib/librte_vhost/vhost_user/virtio-net-user.c
433   rarp = (struct ether_arp *)(eth_hdr + 1);
527   ifr = (struct ifreq *)ifc.ifc_buf;

Regards
Jan

Re: [dpdk-dev] [PATCH 1/1] arm: set CONFIG_RTE_ARCH_STRICT_ALIGN=y for armv7 target

[Jan Viktorin]

On Mon, 29 Feb 2016 16:55:38 +0100
Jan Viktorin <viktorin@rehivetech.com> wrote:

> On Mon, 29 Feb 2016 16:14:58 +0100
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> > 2015-12-09 16:16, Jan Viktorin:  
> > > This patch reduces number of warnings from 53 to 40. It removes the usual false
> > > positives utilizing unaligned_uint*_t data types.
> > > 
> > > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>    
> > 
> > Applied, thanks
> > 
> > Jan, what is the problem with the other ARM alignment warnings?
> > Can they be fixed?  
> 
> This is the full list of warnings I can see on the current origin/master
> for ARMv7 (42 occurences) including examples (+10 more). The origin of
> all of them is:
> 
>   cast increases required alignment of target type [-Wcast-align]
> 
> After skimming through the list, you can see that they are mostly casts
> to uint32_t * or something similar. I believe that all of them are OK.
> However, I don't know how to persuade GCC to not be angry...
> 
> Probably, we can add some explicit alignment of certain structures.
> 
[snip]
> 
> lib/librte_vhost/vhost_user/virtio-net-user.c
> 433   rarp = (struct ether_arp *)(eth_hdr + 1);
> 527   ifr = (struct ifreq *)ifc.ifc_buf;

Fixed recently in
http://dpdk.org/browse/dpdk/commit/?id=bb66588304632a7e4a043d2921d06709d40f9ed4

> 
> Regards
> Jan