[PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[Leonard Crestez]

These fields hold positive errno values which are limited by
ERRNO_MAX=4095 so 16 bits is more than enough.

They are also always positive; setting them to a negative errno value
can result in falsely reporting a successful read/write of incorrect
size.

Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
 include/net/sock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I ran some relatively complex tests without noticing issues but some corner
case where this breaks might exist.

diff --git a/include/net/sock.h b/include/net/sock.h
index 0dd43c3df49b..acd85d1702d9 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -480,11 +480,11 @@ struct sock {
 	u16			sk_protocol;
 	u16			sk_gso_max_segs;
 	unsigned long	        sk_lingertime;
 	struct proto		*sk_prot_creator;
 	rwlock_t		sk_callback_lock;
-	int			sk_err,
+	u16			sk_err,
 				sk_err_soft;
 	u32			sk_ack_backlog;
 	u32			sk_max_ack_backlog;
 	kuid_t			sk_uid;
 	u8			sk_txrehash;
-- 
2.25.1

Re: [PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[Paolo Abeni]

On Sun, 2022-07-03 at 23:06 +0300, Leonard Crestez wrote:
> These fields hold positive errno values which are limited by
> ERRNO_MAX=4095 so 16 bits is more than enough.
> 
> They are also always positive; setting them to a negative errno value
> can result in falsely reporting a successful read/write of incorrect
> size.
> 
> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
> ---
>  include/net/sock.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I ran some relatively complex tests without noticing issues but some corner
> case where this breaks might exist.

Could you please explain in length the rationale behind this change?

Note that this additionally changes the struct sock binary layout,
which in turn in quite relevant for high speed data transfer.

Thanks!

Paolo

Re: [PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[Leonard Crestez]

On 7/5/22 13:31, Paolo Abeni wrote:
> On Sun, 2022-07-03 at 23:06 +0300, Leonard Crestez wrote:
>> These fields hold positive errno values which are limited by
>> ERRNO_MAX=4095 so 16 bits is more than enough.
>>
>> They are also always positive; setting them to a negative errno value
>> can result in falsely reporting a successful read/write of incorrect
>> size.
>>
>> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
>> ---
>>   include/net/sock.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> I ran some relatively complex tests without noticing issues but some corner
>> case where this breaks might exist.
> 
> Could you please explain in length the rationale behind this change?
> 
> Note that this additionally changes the struct sock binary layout,
> which in turn in quite relevant for high speed data transfer.

The rationale is that shrinking structs is almost always better. I know 
that due to various roundings it likely won't actually impact memory 
consumption unless accumulated with other size reductions.

These sk_err fields don't seem to be in a particularly "hot" area so I 
don't think it will impact performance.

My expectation is that after a socket error is reported the socket will 
likely be closed so that there will be very few writes to this field.

--
Regards,
Leonard

Re: [PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[Soheil Hassas Yeganeh]

On Tue, Jul 5, 2022 at 9:01 AM Leonard Crestez <cdleonard@gmail.com> wrote:
>
> On 7/5/22 13:31, Paolo Abeni wrote:
> > On Sun, 2022-07-03 at 23:06 +0300, Leonard Crestez wrote:
> >> These fields hold positive errno values which are limited by
> >> ERRNO_MAX=4095 so 16 bits is more than enough.
> >>
> >> They are also always positive; setting them to a negative errno value
> >> can result in falsely reporting a successful read/write of incorrect
> >> size.
> >>
> >> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
> >> ---
> >>   include/net/sock.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> I ran some relatively complex tests without noticing issues but some corner
> >> case where this breaks might exist.
> >
> > Could you please explain in length the rationale behind this change?
> >
> > Note that this additionally changes the struct sock binary layout,
> > which in turn in quite relevant for high speed data transfer.
>
> The rationale is that shrinking structs is almost always better. I know
> that due to various roundings it likely won't actually impact memory
> consumption unless accumulated with other size reductions.
>
> These sk_err fields don't seem to be in a particularly "hot" area so I
> don't think it will impact performance.
>
> My expectation is that after a socket error is reported the socket will
> likely be closed so that there will be very few writes to this field.

Since you're packing sk_err and sk_err_soft into a DWORD, I'd suggest
adding another patch on top to move both fields right before sk_filter
where we have a 4-byte hole. As far as I can tell, this should save
one QWORD from "struct sock".

Eric, I believe these fields are read-mostly and that wouldn't infer
with your previous layout optimizations. Is my understanding correct?

Thanks,
Soheil

> --
> Regards,
> Leonard

Re: [PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[Jakub Kicinski]

On Sun,  3 Jul 2022 23:06:43 +0300 Leonard Crestez wrote:
> -	int			sk_err,
> +	u16			sk_err,
>  				sk_err_soft;

While at it please remove the comma and explicitly type both fields.

BTW are there are no architectures of note which can't load 2B entities,
any more? Historically 16b is an awkward quantity for RISC arches.

Re: [PATCH] net: Shrink sock.sk_err sk_err_soft to u16 from int

[Eric Dumazet]

On Sun, Jul 3, 2022 at 10:07 PM Leonard Crestez <cdleonard@gmail.com> wrote:
>
> These fields hold positive errno values which are limited by
> ERRNO_MAX=4095 so 16 bits is more than enough.
>
> They are also always positive; setting them to a negative errno value
> can result in falsely reporting a successful read/write of incorrect
> size.
>
> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
> ---

We can not do this safely.

sk->sk_err_soft can be written without lock, this needs to be a full integer,
otherwise this might pollute adjacent bytes.

>  include/net/sock.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I ran some relatively complex tests without noticing issues but some corner
> case where this breaks might exist.
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 0dd43c3df49b..acd85d1702d9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -480,11 +480,11 @@ struct sock {
>         u16                     sk_protocol;
>         u16                     sk_gso_max_segs;
>         unsigned long           sk_lingertime;
>         struct proto            *sk_prot_creator;
>         rwlock_t                sk_callback_lock;
> -       int                     sk_err,
> +       u16                     sk_err,
>                                 sk_err_soft;
>         u32                     sk_ack_backlog;
>         u32                     sk_max_ack_backlog;
>         kuid_t                  sk_uid;
>         u8                      sk_txrehash;
> --
> 2.25.1
>