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

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

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.
  

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?
  

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
  

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?
  

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.
  

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...