Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   Tung Quang Nguyen <tung.q.nguyen@dektech.com.au>
To:     xu <xu.xin.sc@gmail.com>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "jmaloy@redhat.com" <jmaloy@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "tipc-discussion@lists.sourceforge.net" 
        <tipc-discussion@lists.sourceforge.net>,
        "xu.xin16@zte.com.cn" <xu.xin16@zte.com.cn>,
        "yang.yang29@zte.com.cn" <yang.yang29@zte.com.cn>,
        "ying.xue@windriver.com" <ying.xue@windriver.com>,
        "zhang.yunkai@zte.com.cn" <zhang.yunkai@zte.com.cn>
Subject: RE: [RFC PATCH] net/tipc: reduce tipc_node lock holding time in
 tipc_rcv
Thread-Topic: [RFC PATCH] net/tipc: reduce tipc_node lock holding time in
 tipc_rcv
Thread-Index: AQHaHbcmsQ7KURUzRkaicoeZO+zrDrCHRXnwgAAp2oCAAAQK0IABobQAgAAL9yA=
Date:   Fri, 24 Nov 2023 08:28:56 +0000
Message-ID: <DB9PR05MB9078FE84F8244C627FDCFA2888B8A@DB9PR05MB9078.eurprd05.prod.outlook.com>
References: <DB9PR05MB9078EEC976944CACEC531C1388B9A@DB9PR05MB9078.eurprd05.prod.outlook.com>
 <20231124073134.2043605-1-xu.xin16@zte.com.cn>
In-Reply-To: <20231124073134.2043605-1-xu.xin16@zte.com.cn>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?scB+IHF2U5YlBF/9AetdJQ13JD9TgB++W/rdSCeHj6/vT3Zq0C1L3cDCWv+5?=
 =?us-ascii?Q?SkcSuZxoOgRMZxcfFqFa289emLjZHB5VNEq9pnMigaHE2/JIjKWBUU8F+emJ?=
 =?us-ascii?Q?9pH/uhPAIWZH0DRUI5219HkWfwAMRZq5NUFIm/2pIDfJvNFCfoeQbSEt4ZzG?=
 =?us-ascii?Q?dwcm63Kjd3tR09UzRTTO6zR3eA5oDDnwgw2l7eMiVTNp1RYEbSTd8f5ZipEe?=
 =?us-ascii?Q?2hieX+LN6POWmJhKvlvt6WBijHOjCczA7CMG08BHyzAeBrvZFBkejzYKNh/H?=
 =?us-ascii?Q?9v7FOqhS9YIKByT6dm0gU/IBbMK/SdDUR16NhHmDSYfO+Ze1JOJFctywHaeY?=
 =?us-ascii?Q?BJ5MLcEnr05RRfbWCWsbpKaOSWvo5Y/L/CWPsbZHr8Hc+1xGhPSlM92K5RA9?=
 =?us-ascii?Q?Y8Cc/vZmmJPk5gPOK/EVz7jilIhwfpMkdqgPHtN6f2tUn6FGj74bC8iQqxRY?=
 =?us-ascii?Q?YtDz9c/z5S2UEOYHf7ugNIBfYMy6oc/14B3V73KWHj7HNc6EyJtiVEXsugO/?=
 =?us-ascii?Q?UfU3RK6rYaB87rr/AN+7fyA1Xcxa6MC2+YbF3rc0ycc2/K1I4OA6/ZhjQwi9?=
 =?us-ascii?Q?3pvGl26PxDd1cfwQifAMaGcPOIlJCyUvZ+ZELMqU9biV+Pbx/fP0ge7b4T+2?=
 =?us-ascii?Q?KPmvBEk7qRN93orw7WCJrEoEgLnmaK7WdCmCHH7qqKJBbHJFhNsOxL2GLAhR?=
 =?us-ascii?Q?3/Jjkjuv7nLNEUv4UJa1vG2XetewBcCqwdGSTQjPsxRD84dFit/HwX0GZ2wt?=
 =?us-ascii?Q?+f0ZtRjSS/I0x6PUrIAa0rTvXgFcHjuftdI9LwWJ06ovPfTuaJlf+MPObh3i?=
 =?us-ascii?Q?5kCNdCUDEiI/0V5jX+Z7M+W8nXx7T7VBjvUjoO05iHTcKTWmExYT5LjUz1Ii?=
 =?us-ascii?Q?kqB4LwuwQpmIdQf6oTI3pE4hQlqQ+/Hhpz3q9d9Upu8eIW2EiQwiUSCFIbDZ?=
 =?us-ascii?Q?CI+Bnr7P2lBgnJFVenRI2HOYxf75sbU9IEH2T8p0Ebclh8CZp4RH1L3D/hwe?=
 =?us-ascii?Q?YG1qESiZy5ULYzvjdBKVlcmsM5xHrNcVcIlk7gKqthSOKSPvrSpXVFTaNFdC?=
 =?us-ascii?Q?BP9vsmedn0ZIHCRnzTNxqJdqEho6Ud7GbHvw6B1bKbylKVRW5qL0k69KUP+U?=
 =?us-ascii?Q?bgRel840liyKn6x8qEyEq7WutGrXVEjEzPoxQd1eSmIhV8/g3rFLEpFYXFYM?=
 =?us-ascii?Q?gKBZFU3n0q+adEiK35vQVD7Y0xTfKwZ+bDFdSi+KxxUNpNN4qkywcrqaBCUB?=
 =?us-ascii?Q?6EETAhGPhxcn4MP/64vN0sN6YZi8/D6hz6sYszJgT5UCuBwG/c442p42wtJe?=
 =?us-ascii?Q?BAO554TJZmFcHp59S8E1FSW/DrpMMjVKJkR5Aa/AUyYp/QVTw8AjOFhjfk2u?=
 =?us-ascii?Q?les/Wti4Y7XvJ70o0rJm5UZ/ckx8PXD43X03tmMN//Ju5EdZj0bV5yH+dQ1X?=
 =?us-ascii?Q?b6TC5N2P16vMVyuB42CI/iS01u8X1fw90jpXgBNUNO8F9uaX5SMen66GflQo?=
 =?us-ascii?Q?D9sFaxWCsWWTcugxmXdCndNHzDzhuEwKnoCcUZVSFp1hNZodeiLMQ/uitO7U?=
 =?us-ascii?Q?BjY+gmFF8dCu+Q7745u8fRd8elIOHNZLLeraabse265TPDdARgTF2keoQKrN?=
 =?us-ascii?Q?hg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: dektech.com.au
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DB9PR05MB9078.eurprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2105a419-9c55-41ab-0b0d-08dbecc766a0
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Nov 2023 08:28:56.7117
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 1957ea50-0dd8-4360-8db0-c9530df996b2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Ho5WAH3nCSuR8rRCv29TQrJRPlmnZFR66gJNqfdXJnbjmzjZQXevtBHf/gBEDGuUn73ECEMeYlL/BS4PG+OoJyCUPveS/6pxrumjGdIRd4A=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAVPR05MB10448
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Fri, 24 Nov 2023 00:29:14 -0800 (PST)

>Could we please solve the problem mentioned above by adding spinlock(&le->=
lock)?
>

No, you cannot do that. As I said before, the link status (including l->sta=
te) needs to be protected by node lock.
What I showed you were just 2 use cases (link reset/delete). There are more=
 use cases (netlink, transmit path etc) that need proper locks.

>For example:
>
>(BTW, I have tested it, with this change, enabling RPS based on tipc port =
can improve 25% of general throughput)
>
>diff --git a/net/tipc/node.c b/net/tipc/node.c index 3105abe97bb9..470c272=
d798e 100644
>--- a/net/tipc/node.c
>+++ b/net/tipc/node.c
>@@ -1079,12 +1079,16 @@ static void tipc_node_link_down(struct tipc_node *=
n, int bearer_id, bool delete)
>                __tipc_node_link_down(n, &bearer_id, &xmitq, &maddr);
>        } else {
>                /* Defuse pending tipc_node_link_up() */
>+               spin_lock_bh(&le->lock);
>                tipc_link_reset(l);
>+               spin_unlock_bh(&le->lock);
>                tipc_link_fsm_evt(l, LINK_RESET_EVT);
>        }
>        if (delete) {
>+               spin_lock_bh(&le->lock);
>                kfree(l);
>                le->link =3D NULL;
>+               spin_unlock_bh(&le->lock);
>                n->link_cnt--;
>        }
>        trace_tipc_node_link_down(n, true, "node link down or deleted!"); =
@@ -2154,14 +2158,15 @@ void tipc_rcv(struct net *net,
>struct sk_buff *skb, struct tipc_bearer *b)
>        /* Receive packet directly if conditions permit */
>        tipc_node_read_lock(n);
>        if (likely((n->state =3D=3D SELF_UP_PEER_UP) && (usr !=3D TUNNEL_P=
ROTOCOL))) {
>+               tipc_node_read_unlock(n);
>                spin_lock_bh(&le->lock);
>                if (le->link) {
>                        rc =3D tipc_link_rcv(le->link, skb, &xmitq);
>                        skb =3D NULL;
>                }
>                spin_unlock_bh(&le->lock);
>-       }
>-       tipc_node_read_unlock(n);
>+       } else
>+               tipc_node_read_unlock(n);
>
>        /* Check/update node state before receiving */
>        if (unlikely(skb)) {
>@@ -2169,12 +2174,13 @@ void tipc_rcv(struct net *net, struct sk_buff *skb=
, struct tipc_bearer *b)
>                        goto out_node_put;
>                tipc_node_write_lock(n);
>                if (tipc_node_check_state(n, skb, bearer_id, &xmitq)) {
>+                       tipc_node_write_unlock(n);
>                        if (le->link) {
>                                rc =3D tipc_link_rcv(le->link, skb, &xmitq=
);
>                                skb =3D NULL;
>                        }
>-               }
>-               tipc_node_write_unlock(n);
>+               } else
>+                       tipc_node_write_unlock(n);
>        }
>
>        if (unlikely(rc & TIPC_LINK_UP_EVT))