Re: [PATCH] soreuseport: use "unsigned int" in __reuseport_alloc()

Re: [PATCH] soreuseport: use "unsigned int" in __reuseport_alloc()

[Craig Gallek]

On Sun, Apr 2, 2017 at 6:18 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Number of sockets is limited by 16-bit, so 64-bit allocation will never
> happen.
>
> 16-bit ops are the worst code density-wise on x86_64 because of
> additional prefix (66).
So this boils down to a compiled code density vs a
readability/maintainability argument?  I'm not familiar with the 16
bit problem you're referring to, but I'd argue that using the
self-documenting u16 as an input parameter to define the range
expectations is more useful that the micro optimization that this
change may buy you in the assembly of one platform.  Especially given
that this is a rare-use function.

>
> Space savings:
>
>         add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
>         function                                     old     new   delta
>         reuseport_add_sock                           539     536      -3
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
>  net/core/sock_reuseport.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -13,9 +13,9 @@
>
>  static DEFINE_SPINLOCK(reuseport_lock);
>
> -static struct sock_reuseport *__reuseport_alloc(u16 max_socks)
> +static struct sock_reuseport *__reuseport_alloc(unsigned int max_socks)
>  {
> -       size_t size = sizeof(struct sock_reuseport) +
> +       unsigned int size = sizeof(struct sock_reuseport) +
>                       sizeof(struct sock *) * max_socks;
>         struct sock_reuseport *reuse = kzalloc(size, GFP_ATOMIC);
>

Re: [PATCH] soreuseport: use "unsigned int" in __reuseport_alloc()

[Alexey Dobriyan]

On Mon, Apr 3, 2017 at 4:56 PM, Craig Gallek <kraigatgoog@gmail.com> wrote:
> On Sun, Apr 2, 2017 at 6:18 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> Number of sockets is limited by 16-bit, so 64-bit allocation will never
>> happen.
>>
>> 16-bit ops are the worst code density-wise on x86_64 because of
>> additional prefix (66).
> So this boils down to a compiled code density vs a
> readability/maintainability argument?  I'm not familiar with the 16
> bit problem you're referring to, but I'd argue that using the
> self-documenting u16 as an input parameter to define the range
> expectations is more useful that the micro optimization that this
> change may buy you in the assembly of one platform.  Especially given
> that this is a rare-use function.

It's not a problem as in "create trouble".
16-bit operations are the worst on x86_64: they require additional prefix,
compiler often has to extend it to 32-bit to do anything useful
(MOVZX = 1 cycle, 3 bytes) because of cast-everything-to-int
behaviour enabled by the language.

RE: [PATCH] soreuseport: use "unsigned int" in __reuseport_alloc()

[David Laight]

From: Alexey Dobriyan
> Sent: 04 April 2017 12:36
> On Mon, Apr 3, 2017 at 4:56 PM, Craig Gallek <kraigatgoog@gmail.com> wrote:
> > On Sun, Apr 2, 2017 at 6:18 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >> Number of sockets is limited by 16-bit, so 64-bit allocation will never
> >> happen.
> >>
> >> 16-bit ops are the worst code density-wise on x86_64 because of
> >> additional prefix (66).
> > So this boils down to a compiled code density vs a
> > readability/maintainability argument?  I'm not familiar with the 16
> > bit problem you're referring to, but I'd argue that using the
> > self-documenting u16 as an input parameter to define the range
> > expectations is more useful that the micro optimization that this
> > change may buy you in the assembly of one platform.  Especially given
> > that this is a rare-use function.
> 
> It's not a problem as in "create trouble".
> 16-bit operations are the worst on x86_64: they require additional prefix,
> compiler often has to extend it to 32-bit to do anything useful
> (MOVZX = 1 cycle, 3 bytes) because of cast-everything-to-int
> behaviour enabled by the language.

It is worse for almost everything except x86.
Do arithmetic on anything smaller than an 'int' and the compiler
has to generate instructions to mask the result to the smaller size.
On x86 is can often use the 'as if' rule to update only %al (etc).

You really, really don't want to use 'char' or 'short' for local
variables, function arguments or function results.
(If you care about code size or execution time.)

	David

Re: [PATCH] soreuseport: use "unsigned int" in __reuseport_alloc()

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Mon, 3 Apr 2017 01:18:23 +0300

> Number of sockets is limited by 16-bit, so 64-bit allocation will never
> happen.
> 
> 16-bit ops are the worst code density-wise on x86_64 because of
> additional prefix (66).
> 
> Space savings:
> 
> 	add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
> 	function                                     old     new   delta
> 	reuseport_add_sock                           539     536      -3
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

Applied.

[PATCH] soreuseport: use "unsigned int" in __reuseport_alloc()

[Alexey Dobriyan]

Number of sockets is limited by 16-bit, so 64-bit allocation will never
happen.

16-bit ops are the worst code density-wise on x86_64 because of
additional prefix (66).

Space savings:

	add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
	function                                     old     new   delta
	reuseport_add_sock                           539     536      -3

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 net/core/sock_reuseport.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -13,9 +13,9 @@
 
 static DEFINE_SPINLOCK(reuseport_lock);
 
-static struct sock_reuseport *__reuseport_alloc(u16 max_socks)
+static struct sock_reuseport *__reuseport_alloc(unsigned int max_socks)
 {
-	size_t size = sizeof(struct sock_reuseport) +
+	unsigned int size = sizeof(struct sock_reuseport) +
 		      sizeof(struct sock *) * max_socks;
 	struct sock_reuseport *reuse = kzalloc(size, GFP_ATOMIC);