Hi Stephen,

On 21/08/2023 03:06, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the net-next tree got a conflict in:
> 
>   include/net/inet_sock.h
> 
> between commit:
> 
>   f866fbc842de ("ipv4: fix data-races around inet->inet_id")
> 
> from the net tree and commit:
> 
>   c274af224269 ("inet: introduce inet->inet_flags")
> 
> from the net-next tree.

Thank you for this conflict resolution!

I had the same issue on our side with MPTCP tree when syncing -net and
net-next and I resolved it a bit differently. Here my comment on the
patch you used:

> diff --cc include/net/inet_sock.h
> index 491ceb7ebe5d,acbb93d7607a..000000000000
> --- a/include/net/inet_sock.h
> +++ b/include/net/inet_sock.h
> @@@ -218,12 -218,12 +218,12 @@@ struct inet_sock 
>   #define inet_dport		sk.__sk_common.skc_dport
>   #define inet_num		sk.__sk_common.skc_num
>   
> + 	unsigned long		inet_flags;
>   	__be32			inet_saddr;
>   	__s16			uc_ttl;
> - 	__u16			cmsg_flags;
> - 	struct ip_options_rcu __rcu	*inet_opt;
> - 	atomic_t		inet_id;
>   	__be16			inet_sport;
> ++	atomic_t		inet_id;
> + 	struct ip_options_rcu __rcu	*inet_opt;

I first put "inet_opt", then "inet_id" here to avoid a hole of 4 bytes.
So just switching these two lines.

Here is the output of pahole when using your patch:

> struct inet_sock {
>         struct sock                sk __attribute__((__aligned__(8))); /*     0   768 */
>         /* --- cacheline 12 boundary (768 bytes) --- */
>         struct ipv6_pinfo *        pinet6;               /*   768     8 */
>         long unsigned int          inet_flags;           /*   776     8 */
>         __be32                     inet_saddr;           /*   784     4 */
>         __s16                      uc_ttl;               /*   788     2 */
>         __be16                     inet_sport;           /*   790     2 */
>         atomic_t                   inet_id;              /*   792     4 */
> 
>         /* XXX 4 bytes hole, try to pack */
> 
>         struct ip_options_rcu *    inet_opt;             /*   800     8 */
>         __u8                       tos;                  /*   808     1 */
>         __u8                       min_ttl;              /*   809     1 */
>         __u8                       mc_ttl;               /*   810     1 */
>         __u8                       pmtudisc;             /*   811     1 */
>         __u8                       rcv_tos;              /*   812     1 */
>         __u8                       convert_csum;         /*   813     1 */
> 
>         /* XXX 2 bytes hole, try to pack */
> 
>         int                        uc_index;             /*   816     4 */
>         int                        mc_index;             /*   820     4 */
>         __be32                     mc_addr;              /*   824     4 */

And now when "inet_id" is placed after "inet_opt":

> struct inet_sock {
>         struct sock                sk __attribute__((__aligned__(8))); /*     0   768 */
>         /* --- cacheline 12 boundary (768 bytes) --- */
>         struct ipv6_pinfo *        pinet6;               /*   768     8 */
>         long unsigned int          inet_flags;           /*   776     8 */
>         __be32                     inet_saddr;           /*   784     4 */
>         __s16                      uc_ttl;               /*   788     2 */
>         __be16                     inet_sport;           /*   790     2 */
>         struct ip_options_rcu *    inet_opt;             /*   792     8 */
>         atomic_t                   inet_id;              /*   800     4 */
>         __u8                       tos;                  /*   804     1 */
>         __u8                       min_ttl;              /*   805     1 */
>         __u8                       mc_ttl;               /*   806     1 */
>         __u8                       pmtudisc;             /*   807     1 */
>         __u8                       rcv_tos;              /*   808     1 */
>         __u8                       convert_csum;         /*   809     1 */
> 
>         /* XXX 2 bytes hole, try to pack */
> 
>         int                        uc_index;             /*   812     4 */
>         int                        mc_index;             /*   816     4 */
>         __be32                     mc_addr;              /*   820     4 */

I noticed that in Eric's patch for the net tree -- f866fbc842de ("ipv4:
fix data-races around inet->inet_id") -- he moved "inet_id" above, I
guess for a similar reason.

Just in case you are interested by taking my version, I attached the
patch I used and the Rerere cache is available there:

https://github.com/multipath-tcp/mptcp-upstream-rr-cache/commit/e63f34f8

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net