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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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.
On Tue, Jul 5, 2022 at 9:01 AM Leonard Crestez <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
On Sun, Jul 3, 2022 at 10:07 PM Leonard Crestez <[email protected]> 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 <[email protected]>
> ---
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
>