[PATCH iproute2] ss: better 32bit support

[PATCH iproute2] ss: better 32bit support

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Socket cookies are 64bit, even if ss happens to be
a 32bit binary, running on a 64 bit host.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 misc/ss.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 21a4366..ba58894 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -679,9 +679,9 @@ static inline char *sock_addr_get_str(const inet_prefix *prefix)
     return tmp;
 }
 
-static unsigned long cookie_sk_get(uint32_t *cookie)
+static unsigned long long cookie_sk_get(const uint32_t *cookie)
 {
-	return (((unsigned long)cookie[1] << 31) << 1) | cookie[0];
+	return (((unsigned long long)cookie[1] << 31) << 1) | cookie[0];
 }
 
 static const char *sstate_name[] = {

Re: [PATCH iproute2] ss: better 32bit support

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

On Tue, 2015-03-10 at 09:56 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Socket cookies are 64bit, even if ss happens to be
> a 32bit binary, running on a 64 bit host.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  misc/ss.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 21a4366..ba58894 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -679,9 +679,9 @@ static inline char *sock_addr_get_str(const inet_prefix *prefix)
>      return tmp;
>  }
>  
> -static unsigned long cookie_sk_get(uint32_t *cookie)
> +static unsigned long long cookie_sk_get(const uint32_t *cookie)
>  {
> -	return (((unsigned long)cookie[1] << 31) << 1) | cookie[0];
> +	return (((unsigned long long)cookie[1] << 31) << 1) | cookie[0];

Then you should also use << 32 instead of splitting it into two shifts.

Ben.

>  }
>  
>  static const char *sstate_name[] = {

-- 
Ben Hutchings
Q.  Which is the greater problem in the world today, ignorance or apathy?
A.  I don't know and I couldn't care less.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH iproute2] ss: better 32bit support

[Eric Dumazet]

On Sat, 2015-03-14 at 19:19 +0000, Ben Hutchings wrote:
> On Tue, 2015-03-10 at 09:56 -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>


> > -static unsigned long cookie_sk_get(uint32_t *cookie)
> > +static unsigned long long cookie_sk_get(const uint32_t *cookie)
> >  {
> > -	return (((unsigned long)cookie[1] << 31) << 1) | cookie[0];
> > +	return (((unsigned long long)cookie[1] << 31) << 1) | cookie[0];
> 
> Then you should also use << 32 instead of splitting it into two shifts.

Yep, why not, although compiler does not care : no shift at all is
generated anyway.

Since kernel code no longer uses this << 31 trick, we can align ss code.

RE: [PATCH iproute2] ss: better 32bit support

[David Laight]

From: Ben Hutchings
> On Tue, 2015-03-10 at 09:56 -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Socket cookies are 64bit, even if ss happens to be
> > a 32bit binary, running on a 64 bit host.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  misc/ss.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/misc/ss.c b/misc/ss.c
> > index 21a4366..ba58894 100644
> > --- a/misc/ss.c
> > +++ b/misc/ss.c
> > @@ -679,9 +679,9 @@ static inline char *sock_addr_get_str(const inet_prefix *prefix)
> >      return tmp;
> >  }
> >
> > -static unsigned long cookie_sk_get(uint32_t *cookie)
> > +static unsigned long long cookie_sk_get(const uint32_t *cookie)
> >  {
> > -	return (((unsigned long)cookie[1] << 31) << 1) | cookie[0];
> > +	return (((unsigned long long)cookie[1] << 31) << 1) | cookie[0];
> 
> Then you should also use << 32 instead of splitting it into two shifts.

I wondered if the code should be reading the value in the host's natural
endianness?
Then the code might be optimisable to:
	return *(unsigned long long *)cookie;
which rather begs the question as to why cookie is uint32_t[2] instead of
uint64_t.

	David

Re: [PATCH iproute2] ss: better 32bit support

[Eric Dumazet]

On Mon, 2015-03-16 at 16:39 +0000, David Laight wrote:

> I wondered if the code should be reading the value in the host's natural
> endianness?
> Then the code might be optimisable to:
> 	return *(unsigned long long *)cookie;

This will trap on arches requesting 8 bytes alignment.

Not all linux hosts run x86

Look at rta_getattr_u64(), and you'll see that manipulating 64bit values
in netlink requires a memcpy() because we have no 64bit alignment
guarantee.

> which rather begs the question as to why cookie is uint32_t[2] instead of
> uint64_t.
> 

This question comes too late, inet_diag was added years ago,
and we must keep ABI compatibility forever.

RE: [PATCH iproute2] ss: better 32bit support

[David Laight]

From: Eric 
> On Mon, 2015-03-16 at 16:39 +0000, David Laight wrote:
> 
> > I wondered if the code should be reading the value in the host's natural
> > endianness?
> > Then the code might be optimisable to:
> > 	return *(unsigned long long *)cookie;
> 
> This will trap on arches requesting 8 bytes alignment.

That is why I wrote 'might be optimisable to' :-)

If the 'cookie' were always processed 'host endian' then it
could be accessed using a structure that contains a 64bit member
that has the __aligned(4) attribute.
On x86 (etc) this would generate a 64bit access, on sparc (etc) the compiler
would generate the required shifts.

> Look at rta_getattr_u64(), and you'll see that manipulating 64bit values
> in netlink requires a memcpy() because we have no 64bit alignment
> guarantee.

Using memcpy() isn't necessarily enough.
If you have a 'u64 *' pointer you can't get gcc to 'forget' that it
must have 8 byte alignment, so it will optimise the memcpy back to
a 64bit word copy.

	David