| Message ID | 74c6f54cd3869258f4c83b46d9e5b95f7f0dab4b.1656878516.git.cdleonard@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Shrink sock.sk_err sk_err_soft to u16 from int | expand |
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
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
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 <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
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 >
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;
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.