* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2020-01-07 17:24 Mat Martineau
0 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2020-01-07 17:24 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
On Tue, 7 Jan 2020, Paolo Abeni wrote:
> On Tue, 2020-01-07 at 16:08 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 07/01/2020 15:05, Paolo Abeni wrote:
>>> We need to update the ebpf accessors for both sk_type and
>>> sk_protocol. The code previously in place has some special
>>> hack to take care such fields being part of a bitfield.
>>>
>>> The fix actually clean-up a bit the existing code.
>>
>> Thank you for looking at that!
>>
>> I have 3 questions below.
>>
>>> Note: I'm not 100% sure about dropping the 'target_size' handling.
>>> This changes the 'target_size' from 1 to 2 for sk_protocol.
>>> Should not matter, as the ebpf target field is 4 bytes wide.
It looks like your handling of target_size is consistent with other 16-bit
fields (like skc_dport), so it seems like the right way to go.
>>>
>>> To be appended to the orig commit message:
>>> """
>>> Take care of BPF field access for sk_type/sk_protocol.
>>> Both of them are now outside the bitfield, so we can use
>>> load instructions without fourther shifting/masking.
>>
>> (detail: s/fourther/further)
>>
>>> v5 -> v6:
>>> - update eBPF accessors, too.
>>> """
>>>
>>> Additionally, Eric's Reviewed-by should be dropped.
>>
>> Good point!
>>
>>> ---
>>> include/net/sock.h | 15 ------------
>>> net/core/filter.c | 58 ++++++++++++++++------------------------------
>>> 2 files changed, 20 insertions(+), 53 deletions(-)
>>>
...
>
> I'll send v1 with the cleanup hinted by the 2nd question after doing
> some tests.
>
I don't have anything to add to Matthieu's comments for now, and thanks
for tracking this down Paolo! Will watch for next version.
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2020-01-07 15:21 Paolo Abeni
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2020-01-07 15:21 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 7447 bytes --]
On Tue, 2020-01-07 at 16:08 +0100, Matthieu Baerts wrote:
> Hi Paolo,
>
> On 07/01/2020 15:05, Paolo Abeni wrote:
> > We need to update the ebpf accessors for both sk_type and
> > sk_protocol. The code previously in place has some special
> > hack to take care such fields being part of a bitfield.
> >
> > The fix actually clean-up a bit the existing code.
>
> Thank you for looking at that!
>
> I have 3 questions below.
>
> > Note: I'm not 100% sure about dropping the 'target_size' handling.
> > This changes the 'target_size' from 1 to 2 for sk_protocol.
> > Should not matter, as the ebpf target field is 4 bytes wide.
> >
> > To be appended to the orig commit message:
> > """
> > Take care of BPF field access for sk_type/sk_protocol.
> > Both of them are now outside the bitfield, so we can use
> > load instructions without fourther shifting/masking.
>
> (detail: s/fourther/further)
>
> > v5 -> v6:
> > - update eBPF accessors, too.
> > """
> >
> > Additionally, Eric's Reviewed-by should be dropped.
>
> Good point!
>
> > ---
> > include/net/sock.h | 15 ------------
> > net/core/filter.c | 58 ++++++++++++++++------------------------------
> > 2 files changed, 20 insertions(+), 53 deletions(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index c63e9088b4e3..8766f9bc3e70 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -436,21 +436,6 @@ struct sock {
> > * Because of non atomicity rules, all
> > * changes are protected by socket lock.
> > */
> > - unsigned int __sk_flags_offset[0];
> > -#ifdef __BIG_ENDIAN_BITFIELD
> > -#define SK_FL_PROTO_SHIFT 16
> > -#define SK_FL_PROTO_MASK 0x00ff0000
> > -
> > -#define SK_FL_TYPE_SHIFT 0
> > -#define SK_FL_TYPE_MASK 0x0000ffff
> > -#else
> > -#define SK_FL_PROTO_SHIFT 8
> > -#define SK_FL_PROTO_MASK 0x0000ff00
> > -
> > -#define SK_FL_TYPE_SHIFT 16
> > -#define SK_FL_TYPE_MASK 0xffff0000
> > -#endif
> > -
> > u8 sk_padding : 1,
> > sk_kern_sock : 1,
> > sk_no_check_tx : 1,
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 42fd17c48c5f..37759d6ce3ea 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -7607,21 +7607,19 @@ u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
> > break;
> >
> > case offsetof(struct bpf_sock, type):
> > - BUILD_BUG_ON(HWEIGHT32(SK_FL_TYPE_MASK) != BITS_PER_BYTE * 2);
>
> Should we not keep it?
>
> I see that some others are still doing that:
>
> BUILD_BUG_ON(sizeof_field(struct sock, sk_mark) != 4);
The first BUILD_BUG reported above regards only the type mask. This
patch remove such definition, so we can't keep the build bug as-is.
The second one instead is still present, via the 'bpf_target_off()'
macro call.
> But not all, e.g. for sk_state, sk_num, sk_dport.
>
> > - *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - offsetof(struct sock, __sk_flags_offset));
> > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_TYPE_MASK);
> > - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, SK_FL_TYPE_SHIFT);
> > - *target_size = 2;
> > + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>
> Instead of BPF_H, should we use this macro?
>
> BPF_FIELD_SIZEOF(struct sock, sk_type)
Right u r, its' more consistent with the nearby code.
> > + bpf_target_off(struct sock, sk_type,
> > + sizeof_field(struct sock,
> > + sk_type),
> > + target_size));
> > break;
> >
> > case offsetof(struct bpf_sock, protocol):
> > - BUILD_BUG_ON(HWEIGHT32(SK_FL_PROTO_MASK) != BITS_PER_BYTE);
> > - *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - offsetof(struct sock, __sk_flags_offset));
> > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
> > - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, SK_FL_PROTO_SHIFT);
> > - *target_size = 1;
> > + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
> > + bpf_target_off(struct sock, sk_protocol,
> > + sizeof_field(struct sock,
> > + sk_protocol),
> > + target_size));
> > break;
> >
> > case offsetof(struct bpf_sock, src_ip4):
> > @@ -7903,20 +7901,13 @@ static u32 sock_addr_convert_ctx_access(enum bpf_access_type type,
> > break;
> >
> > case offsetof(struct bpf_sock_addr, type):
> > - SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(
> > - struct bpf_sock_addr_kern, struct sock, sk,
> > - __sk_flags_offset, BPF_W, 0);
> > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_TYPE_MASK);
> > - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, SK_FL_TYPE_SHIFT);
> > + SOCK_ADDR_LOAD_NESTED_FIELD(struct bpf_sock_addr_kern,
> > + struct sock, sk, sk_type);
> > break;
> >
> > case offsetof(struct bpf_sock_addr, protocol):
> > - SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(
> > - struct bpf_sock_addr_kern, struct sock, sk,
> > - __sk_flags_offset, BPF_W, 0);
> > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
> > - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg,
> > - SK_FL_PROTO_SHIFT);
> > + SOCK_ADDR_LOAD_NESTED_FIELD(struct bpf_sock_addr_kern,
> > + struct sock, sk, sk_protocol);
> > break;
> >
> > case offsetof(struct bpf_sock_addr, msg_src_ip4):
> > @@ -8835,11 +8826,11 @@ sk_reuseport_is_valid_access(int off, int size,
> > skb, \
> > SKB_FIELD)
> >
> > -#define SK_REUSEPORT_LOAD_SK_FIELD_SIZE_OFF(SK_FIELD, BPF_SIZE, EXTRA_OFF) \
> > - SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(struct sk_reuseport_kern, \
> > - struct sock, \
> > - sk, \
> > - SK_FIELD, BPF_SIZE, EXTRA_OFF)
> > +#define SK_REUSEPORT_LOAD_SK_FIELD(SK_FIELD) \
> > + SOCK_ADDR_LOAD_NESTED_FIELD(struct sk_reuseport_kern, \
> > + struct sock, \
> > + sk, \
> > + SK_FIELD)
>
> Why did you have to change that too?
SK_REUSEPORT_LOAD_SK_FIELD_SIZE_OFF() is unused after this patch.
So I crafted another helper to make sk_protocol access easier.
>
> >
> > static u32 sk_reuseport_convert_ctx_access(enum bpf_access_type type,
> > const struct bpf_insn *si,
> > @@ -8863,16 +8854,7 @@ static u32 sk_reuseport_convert_ctx_access(enum bpf_access_type type,
> > break;
> >
> > case offsetof(struct sk_reuseport_md, ip_protocol):
> > - BUILD_BUG_ON(HWEIGHT32(SK_FL_PROTO_MASK) != BITS_PER_BYTE);
> > - SK_REUSEPORT_LOAD_SK_FIELD_SIZE_OFF(__sk_flags_offset,
> > - BPF_W, 0);
> > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
> > - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg,
> > - SK_FL_PROTO_SHIFT);
> > - /* SK_FL_PROTO_MASK and SK_FL_PROTO_SHIFT are endian
> > - * aware. No further narrowing or masking is needed.
> > - */
> > - *target_size = 1;
> > + SK_REUSEPORT_LOAD_SK_FIELD(sk_protocol);
>
> Detail for me: why did they not modify the bitfield instead of adding
> __sk_flags_offset[0] in include/net/sock.h ?
I guess to avoid touching a long established and delicate piece of
code. You know, you get quite serious feedback when doint that kind of
changes ;)
I'll send v1 with the cleanup hinted by the 2nd question after doing
some tests.
/P
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2020-01-07 15:08 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2020-01-07 15:08 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 6576 bytes --]
Hi Paolo,
On 07/01/2020 15:05, Paolo Abeni wrote:
> We need to update the ebpf accessors for both sk_type and
> sk_protocol. The code previously in place has some special
> hack to take care such fields being part of a bitfield.
>
> The fix actually clean-up a bit the existing code.
Thank you for looking at that!
I have 3 questions below.
> Note: I'm not 100% sure about dropping the 'target_size' handling.
> This changes the 'target_size' from 1 to 2 for sk_protocol.
> Should not matter, as the ebpf target field is 4 bytes wide.
>
> To be appended to the orig commit message:
> """
> Take care of BPF field access for sk_type/sk_protocol.
> Both of them are now outside the bitfield, so we can use
> load instructions without fourther shifting/masking.
(detail: s/fourther/further)
>
> v5 -> v6:
> - update eBPF accessors, too.
> """
>
> Additionally, Eric's Reviewed-by should be dropped.
Good point!
> ---
> include/net/sock.h | 15 ------------
> net/core/filter.c | 58 ++++++++++++++++------------------------------
> 2 files changed, 20 insertions(+), 53 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index c63e9088b4e3..8766f9bc3e70 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -436,21 +436,6 @@ struct sock {
> * Because of non atomicity rules, all
> * changes are protected by socket lock.
> */
> - unsigned int __sk_flags_offset[0];
> -#ifdef __BIG_ENDIAN_BITFIELD
> -#define SK_FL_PROTO_SHIFT 16
> -#define SK_FL_PROTO_MASK 0x00ff0000
> -
> -#define SK_FL_TYPE_SHIFT 0
> -#define SK_FL_TYPE_MASK 0x0000ffff
> -#else
> -#define SK_FL_PROTO_SHIFT 8
> -#define SK_FL_PROTO_MASK 0x0000ff00
> -
> -#define SK_FL_TYPE_SHIFT 16
> -#define SK_FL_TYPE_MASK 0xffff0000
> -#endif
> -
> u8 sk_padding : 1,
> sk_kern_sock : 1,
> sk_no_check_tx : 1,
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 42fd17c48c5f..37759d6ce3ea 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7607,21 +7607,19 @@ u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
> break;
>
> case offsetof(struct bpf_sock, type):
> - BUILD_BUG_ON(HWEIGHT32(SK_FL_TYPE_MASK) != BITS_PER_BYTE * 2);
Should we not keep it?
I see that some others are still doing that:
BUILD_BUG_ON(sizeof_field(struct sock, sk_mark) != 4);
But not all, e.g. for sk_state, sk_num, sk_dport.
> - *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> - offsetof(struct sock, __sk_flags_offset));
> - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_TYPE_MASK);
> - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, SK_FL_TYPE_SHIFT);
> - *target_size = 2;
> + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
Instead of BPF_H, should we use this macro?
BPF_FIELD_SIZEOF(struct sock, sk_type)
> + bpf_target_off(struct sock, sk_type,
> + sizeof_field(struct sock,
> + sk_type),
> + target_size));
> break;
>
> case offsetof(struct bpf_sock, protocol):
> - BUILD_BUG_ON(HWEIGHT32(SK_FL_PROTO_MASK) != BITS_PER_BYTE);
> - *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> - offsetof(struct sock, __sk_flags_offset));
> - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
> - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, SK_FL_PROTO_SHIFT);
> - *target_size = 1;
> + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
> + bpf_target_off(struct sock, sk_protocol,
> + sizeof_field(struct sock,
> + sk_protocol),
> + target_size));
> break;
>
> case offsetof(struct bpf_sock, src_ip4):
> @@ -7903,20 +7901,13 @@ static u32 sock_addr_convert_ctx_access(enum bpf_access_type type,
> break;
>
> case offsetof(struct bpf_sock_addr, type):
> - SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(
> - struct bpf_sock_addr_kern, struct sock, sk,
> - __sk_flags_offset, BPF_W, 0);
> - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_TYPE_MASK);
> - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, SK_FL_TYPE_SHIFT);
> + SOCK_ADDR_LOAD_NESTED_FIELD(struct bpf_sock_addr_kern,
> + struct sock, sk, sk_type);
> break;
>
> case offsetof(struct bpf_sock_addr, protocol):
> - SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(
> - struct bpf_sock_addr_kern, struct sock, sk,
> - __sk_flags_offset, BPF_W, 0);
> - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
> - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg,
> - SK_FL_PROTO_SHIFT);
> + SOCK_ADDR_LOAD_NESTED_FIELD(struct bpf_sock_addr_kern,
> + struct sock, sk, sk_protocol);
> break;
>
> case offsetof(struct bpf_sock_addr, msg_src_ip4):
> @@ -8835,11 +8826,11 @@ sk_reuseport_is_valid_access(int off, int size,
> skb, \
> SKB_FIELD)
>
> -#define SK_REUSEPORT_LOAD_SK_FIELD_SIZE_OFF(SK_FIELD, BPF_SIZE, EXTRA_OFF) \
> - SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(struct sk_reuseport_kern, \
> - struct sock, \
> - sk, \
> - SK_FIELD, BPF_SIZE, EXTRA_OFF)
> +#define SK_REUSEPORT_LOAD_SK_FIELD(SK_FIELD) \
> + SOCK_ADDR_LOAD_NESTED_FIELD(struct sk_reuseport_kern, \
> + struct sock, \
> + sk, \
> + SK_FIELD)
Why did you have to change that too?
>
> static u32 sk_reuseport_convert_ctx_access(enum bpf_access_type type,
> const struct bpf_insn *si,
> @@ -8863,16 +8854,7 @@ static u32 sk_reuseport_convert_ctx_access(enum bpf_access_type type,
> break;
>
> case offsetof(struct sk_reuseport_md, ip_protocol):
> - BUILD_BUG_ON(HWEIGHT32(SK_FL_PROTO_MASK) != BITS_PER_BYTE);
> - SK_REUSEPORT_LOAD_SK_FIELD_SIZE_OFF(__sk_flags_offset,
> - BPF_W, 0);
> - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, SK_FL_PROTO_MASK);
> - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg,
> - SK_FL_PROTO_SHIFT);
> - /* SK_FL_PROTO_MASK and SK_FL_PROTO_SHIFT are endian
> - * aware. No further narrowing or masking is needed.
> - */
> - *target_size = 1;
> + SK_REUSEPORT_LOAD_SK_FIELD(sk_protocol);
Detail for me: why did they not modify the bitfield instead of adding
__sk_flags_offset[0] in include/net/sock.h ?
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-17 11:55 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2019-12-17 11:55 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]
Hi Paolo,
On 17/12/2019 12:08, Paolo Abeni wrote:
> On Tue, 2019-12-17 at 11:02 +0100, Matthieu Baerts wrote:
>> move sk_type to a separate field to:
>> - avoid extra operations to fetch sk->sk_type on some arches (Eric)
>> - allow compilers to emit better code to access/manipulate native types
>> compared to bitfields. (Paolo)
>>
>> Suggested-by: Eric Dumazet <edumazet(a)google.com>
>> Suggested-by: Paolo Abeni <pabeni(a)redhat.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
>> ---
>>
>> Notes:
>> to be squashed in "sock: Make sk_protocol a 16-bit value"
>>
>> include/net/sock.h | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 0930f46c600c..b93cadba1a3b 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -451,13 +451,13 @@ struct sock {
>> #define SK_FL_TYPE_MASK 0xffff0000
>> #endif
>>
>> - unsigned int sk_padding : 1,
>> + u8 sk_padding : 1,
>> sk_kern_sock : 1,
>> sk_no_check_tx : 1,
>> sk_no_check_rx : 1,
>> - sk_userlocks : 4,
>> - sk_type : 16;
>> + sk_userlocks : 4;
>> u8 sk_pacing_shift;
>> + u16 sk_type;
>> u16 sk_protocol;
>> u16 sk_gso_max_segs;
>> unsigned long sk_lingertime;
>
> LGTM.
Thank you for the review and the suggestion not to simply move it to the
beginning of the 'unsigned int' but to move it to a separated field.
- 32121b5387c7: "squashed" in "sock: Make sk_protocol a 16-bit value"
- 1e3599433335: "Signed-off-by" + "Co-developed-by"
- 3a1b579608d1..bf07050cd31b: result
Tests are still OK!
> @list, please do not reply directly to the orig messages, or trim the
> recipients list, as eric email address unintentionally slipped into it.
Sorry for that :(
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-17 11:08 Paolo Abeni
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2019-12-17 11:08 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]
On Tue, 2019-12-17 at 11:02 +0100, Matthieu Baerts wrote:
> move sk_type to a separate field to:
> - avoid extra operations to fetch sk->sk_type on some arches (Eric)
> - allow compilers to emit better code to access/manipulate native types
> compared to bitfields. (Paolo)
>
> Suggested-by: Eric Dumazet <edumazet(a)google.com>
> Suggested-by: Paolo Abeni <pabeni(a)redhat.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> ---
>
> Notes:
> to be squashed in "sock: Make sk_protocol a 16-bit value"
>
> include/net/sock.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 0930f46c600c..b93cadba1a3b 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -451,13 +451,13 @@ struct sock {
> #define SK_FL_TYPE_MASK 0xffff0000
> #endif
>
> - unsigned int sk_padding : 1,
> + u8 sk_padding : 1,
> sk_kern_sock : 1,
> sk_no_check_tx : 1,
> sk_no_check_rx : 1,
> - sk_userlocks : 4,
> - sk_type : 16;
> + sk_userlocks : 4;
> u8 sk_pacing_shift;
> + u16 sk_type;
> u16 sk_protocol;
> u16 sk_gso_max_segs;
> unsigned long sk_lingertime;
LGTM.
@list, please do not reply directly to the orig messages, or trim the
recipients list, as eric email address unintentionally slipped into it.
/P
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-16 18:41 Mat Martineau
0 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2019-12-16 18:41 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]
On Mon, 16 Dec 2019, Paolo Abeni wrote:
> On Mon, 2019-12-16 at 08:51 -0800, Mat Martineau wrote:
>> On Mon, 16 Dec 2019, Paolo Abeni wrote:
>>
>>> move sk_pacing_shift to a separate field, so we can READ_ONCE() it.
>>>
>>> Binary layout of struct sock dose not change (except for the
>>> sk_pacing_shift position)
>>> ---
>>> This addresses Eric's upstream feedback on v1
>>> ---
>>> include/net/sock.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/sock.h b/include/net/sock.h
>>> index 9dd225f62012..0930f46c600c 100644
>>> --- a/include/net/sock.h
>>> +++ b/include/net/sock.h
>>> @@ -456,8 +456,8 @@ struct sock {
>>> sk_no_check_tx : 1,
>>> sk_no_check_rx : 1,
>>> sk_userlocks : 4,
>>> - sk_pacing_shift : 8,
>>> sk_type : 16;
>>> + u8 sk_pacing_shift;
>>> u16 sk_protocol;
>>> u16 sk_gso_max_segs;
>>
>> sk_pacing_shift was here (between sk_gso_max_segs and sk_lingertime)
>> before, would be good to put it back where it was for a clean diff.
>
> But we can't move back there, otherwise the 'struct sock' will grow
> quite a bit (we will have 7 bytes hole on 64 bits arch).
>
> With the proposed patch 'struct sock' size does not change.
>
I didn't know gcc was smart enough to pack the u8 in with the previous
bitfield! Learned something new.
I think Paolo's fix is ok as-is, unless it makes sense to worry about
2-byte alignment of sk_type. It does appear to land on an odd byte now
(checked with objdump).
Thanks
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-16 18:29 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2019-12-16 18:29 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]
Hi Paolo, Mat,
On 16/12/2019 18:47, Paolo Abeni wrote:
> On Mon, 2019-12-16 at 08:51 -0800, Mat Martineau wrote:
>> On Mon, 16 Dec 2019, Paolo Abeni wrote:
>>
>>> move sk_pacing_shift to a separate field, so we can READ_ONCE() it.
>>>
>>> Binary layout of struct sock dose not change (except for the
>>> sk_pacing_shift position)
>>> ---
>>> This addresses Eric's upstream feedback on v1
>>> ---
>>> include/net/sock.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/sock.h b/include/net/sock.h
>>> index 9dd225f62012..0930f46c600c 100644
>>> --- a/include/net/sock.h
>>> +++ b/include/net/sock.h
>>> @@ -456,8 +456,8 @@ struct sock {
>>> sk_no_check_tx : 1,
>>> sk_no_check_rx : 1,
>>> sk_userlocks : 4,
>>> - sk_pacing_shift : 8,
>>> sk_type : 16;
>>> + u8 sk_pacing_shift;
>>> u16 sk_protocol;
>>> u16 sk_gso_max_segs;
>>
>> sk_pacing_shift was here (between sk_gso_max_segs and sk_lingertime)
>> before, would be good to put it back where it was for a clean diff.
>
> But we can't move back there, otherwise the 'struct sock' will grow
> quite a bit (we will have 7 bytes hole on 64 bits arch).
>
> With the proposed patch 'struct sock' size does not change.
- 548181045475: "squashed" in "sock: Make sk_protocol a 16-bit value"
- 4d09491bafaf: "Signed-off-by" + "Co-developed-by"
- 66880827b48f..e999bb17232e: result
Tests + export will be launched soon.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-16 17:47 Paolo Abeni
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2019-12-16 17:47 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]
On Mon, 2019-12-16 at 08:51 -0800, Mat Martineau wrote:
> On Mon, 16 Dec 2019, Paolo Abeni wrote:
>
> > move sk_pacing_shift to a separate field, so we can READ_ONCE() it.
> >
> > Binary layout of struct sock dose not change (except for the
> > sk_pacing_shift position)
> > ---
> > This addresses Eric's upstream feedback on v1
> > ---
> > include/net/sock.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 9dd225f62012..0930f46c600c 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -456,8 +456,8 @@ struct sock {
> > sk_no_check_tx : 1,
> > sk_no_check_rx : 1,
> > sk_userlocks : 4,
> > - sk_pacing_shift : 8,
> > sk_type : 16;
> > + u8 sk_pacing_shift;
> > u16 sk_protocol;
> > u16 sk_gso_max_segs;
>
> sk_pacing_shift was here (between sk_gso_max_segs and sk_lingertime)
> before, would be good to put it back where it was for a clean diff.
But we can't move back there, otherwise the 'struct sock' will grow
quite a bit (we will have 7 bytes hole on 64 bits arch).
With the proposed patch 'struct sock' size does not change.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-16 16:51 Mat Martineau
0 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2019-12-16 16:51 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 987 bytes --]
On Mon, 16 Dec 2019, Paolo Abeni wrote:
> move sk_pacing_shift to a separate field, so we can READ_ONCE() it.
>
> Binary layout of struct sock dose not change (except for the
> sk_pacing_shift position)
> ---
> This addresses Eric's upstream feedback on v1
> ---
> include/net/sock.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 9dd225f62012..0930f46c600c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -456,8 +456,8 @@ struct sock {
> sk_no_check_tx : 1,
> sk_no_check_rx : 1,
> sk_userlocks : 4,
> - sk_pacing_shift : 8,
> sk_type : 16;
> + u8 sk_pacing_shift;
> u16 sk_protocol;
> u16 sk_gso_max_segs;
sk_pacing_shift was here (between sk_gso_max_segs and sk_lingertime)
before, would be good to put it back where it was for a clean diff.
> unsigned long sk_lingertime;
> --
> 2.21.0
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value"
@ 2019-12-16 13:13 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2019-12-16 13:13 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 979 bytes --]
Hi Paolo,
On 16/12/2019 13:31, Paolo Abeni wrote:
> move sk_pacing_shift to a separate field, so we can READ_ONCE() it.
Thank you addressing Eric's comments!
> Binary layout of struct sock dose not change (except for the
> sk_pacing_shift position)
Just to be sure, does the compiler will optimize the structure to remove
the "hole" we have here? We use 24/32 bits of the "unsigned int" and
then we declare the u8. But because we don't declare the "__packed__"
attribute, the compiler will do his job and we don't increase the size.
Right? :)
> ---
> This addresses Eric's upstream feedback on v1
I guess we should add this in the commit message.
Or:
v1:
- move sk_pacing_shift to a separate field, so we can READ_ONCE() it
(Eric Dumazet)
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-07 17:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 17:24 [MPTCP] Re: [PATCH] Squash-to: "sock: Make sk_protocol a 16-bit value" Mat Martineau
-- strict thread matches above, loose matches on Subject: below --
2020-01-07 15:21 Paolo Abeni
2020-01-07 15:08 Matthieu Baerts
2019-12-17 11:55 Matthieu Baerts
2019-12-17 11:08 Paolo Abeni
2019-12-16 18:41 Mat Martineau
2019-12-16 18:29 Matthieu Baerts
2019-12-16 17:47 Paolo Abeni
2019-12-16 16:51 Mat Martineau
2019-12-16 13:13 Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.