Re: [PATCH net-next] tcp: replace per-packet memset in __tcp_transmit_skb()

[Keita Morisaki <kmta1236@gmail.com>]

> Interesting idea, but the implementation is a bit fragile.
>
> I would instead add a group in struct tcp_out_options to clearly mark
> which group is zeroed in  __tcp_transmit_skb().

That's a great idea.

>
> Partial patch to give the idea :
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 46bd48cf776a6a37e6ab2664245cd1e35a88d4f8..b96168f50170b7359d26de2389128bc80d4d549e
> 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -429,14 +429,19 @@ static void smc_options_write(__be32 *ptr, u16 *options)
>  }
>
>  struct tcp_out_options {
> +       /* Following group is cleared in __tcp_transmit_skb() */
> +       struct_group(cleared,
> +               u16 mss;                /* 0 to disable */
> +               u8 bpf_opt_len;         /* length of BPF hdr option */
> +               u8 num_sack_blocks;     /* number of SACK blocks to include */
> +       );
> +
> +       /* Caution : Following fields are not cleared in
> __tcp_transmit_skb(). */
>         u16 options;            /* bit field of OPTION_* */
> -       u16 mss;                /* 0 to disable */
>         u8 ws;                  /* window scale, 0 to disable */
> -       u8 num_sack_blocks;     /* number of SACK blocks to include */
>         u8 num_accecn_fields:7, /* number of AccECN fields needed */
>            use_synack_ecn_bytes:1; /* Use synack_ecn_bytes or not */
>         u8 hash_size;           /* bytes in hash_location */
> -       u8 bpf_opt_len;         /* length of BPF hdr option */
>         __u8 *hash_location;    /* temporary pointer, overloaded */
>         __u32 tsval, tsecr;     /* need to include OPTION_TS */
>         struct tcp_fastopen_cookie *fastopen_cookie;    /* Fast open cookie */
> @@ -1565,7 +1570,7 @@ static int __tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb,
>
>         inet = inet_sk(sk);
>         tcb = TCP_SKB_CB(skb);
> -       memset(&opts, 0, sizeof(opts));
> +       memset(&opts.cleared, 0, sizeof(opts.cleared));

Will send V2 with this change. Thank you for the review!