Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9d4e37e2-417e-7a2f-a187-5c7b680c6777@intel.com>
Date:   Tue, 2 Aug 2022 14:36:00 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [LKP] Re: [xfs] 016a23388c: stress-ng.xattr.ops_per_sec 58.4%
 improvement
Content-Language: en-US
To:     Dave Chinner <david@fromorbit.com>,
        kernel test robot <oliver.sang@intel.com>
CC:     Dave Chinner <dchinner@redhat.com>,
        "Darrick J. Wong" <djwong@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        <linux-xfs@vger.kernel.org>, <lkp@lists.01.org>, <lkp@intel.com>
References: <Yti6PccitrglBtIj@xsang-OptiPlex-9020>
 <20220721220142.GW3861211@dread.disaster.area>
From:   "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <20220721220142.GW3861211@dread.disaster.area>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Tk15QnUzVnJmRExVNk5WSjhCVFFKNGhwTDUrbTZ6cFEweVJ5d3pKUU9ucTRr?=
 =?utf-8?B?QWQ1ZjFIM2kvVVI2MnMrOFRWa3U2WStIbnR0elZnWVEwSUFFSW1WSVV1c3Fu?=
 =?utf-8?B?NXQ0dHpDTlV3YVU1UHVPWU5uTit3b215K2FkNTBGTVNOUkc3anNLTlNVME44?=
 =?utf-8?B?MzNrN3ExcHRCQ1NCaGk0UlRGcnpheUxQdkx2NER1RDJGMHd4ZE9UL2tCODRj?=
 =?utf-8?B?R1hJN2YzM1JhcVdmdyt5d05PQVloUi9xT0hqeG5sRk85bHJHUXUxRVRxekdC?=
 =?utf-8?B?UzkyZlIrTmNzRGZrbVFwY1BUNlBTZkRPTHN5NEp3MS9qSGNKek5tY2FObWtM?=
 =?utf-8?B?Tkx0U2N6YmxZTFVkWWxMeEt4NndHc3pINVV5QitmVlM1dTN3N1VqbFZMNFVN?=
 =?utf-8?B?KzBzM0dRbWVYRWlqWG84blJOcldmR3dEcnB3Kzl6N0pGNXoycmxGakpCeHA5?=
 =?utf-8?B?RkhBMUhFblRoMitFNnkrRG1UVUxkOEhCTTMyNzZuUEQ5RkVDRmk3OGR0UnRv?=
 =?utf-8?B?ZHFON3J5ZnFNYk4ySHJrcWljbzAvS09HS2NrNG5WMnBHSjQzT295dlZab0dT?=
 =?utf-8?B?Y0VjRGVlRm0yUlQzd2dlNlA2T3NzTDdyeU1QNTlpWnBianRFNkZ3enR5elFX?=
 =?utf-8?B?ZkJFUUUwb2RjVDhvOVBjdS9NTW5sTjUxMTVYd0xWR3pTUDZJL2s3SU5XK1R4?=
 =?utf-8?B?UXNWRmVKZzhYS1lNTGUvb0dQNGJOU2hlNXdqQW4vOTVmU0ZtTkwzZzFaaElF?=
 =?utf-8?B?OWhZTzZ0alBtbTU5alZjUVA0NGxGM2FJblloWEd3cnc4UnEzQVpxa2NKbHBH?=
 =?utf-8?B?NFVKUlZ1VGkzSmdDVWhBeGJPRDNFZ2tBR3VRL2U5dldkUzQ1V2RiSktmWGRS?=
 =?utf-8?B?UEZsN3RaUnRJbXlUY0piWFNkTSt1TlBEN3A5OFp0TkhpYlBxVFFtU0pDNFZS?=
 =?utf-8?B?ZlZxMWxtV2psOFZxWWVsVFBjZGVrSlcyUkxVcEJrekxWVTJ5N1pVY2NNVWVS?=
 =?utf-8?B?Rm1LUVV1UENSdEdsQmJCZ0tDb2FvYmo0b3cvZDVGWmFkdXJrNTF1ZFhsWHZM?=
 =?utf-8?B?Q3J0bGlaeXRESk9BWEZRaG9ieElSNWtiUW16Qlp2MkNZM1B1a3BNZkZ0Yk82?=
 =?utf-8?B?bnRlOE1Vcm9UUnpoTnZyQ2s3Y0pHNUI4KzArS3J5OGthZ1psS3VmdzR5M1Zs?=
 =?utf-8?B?U0J5YTBIVmZwTG41UVNJbi9mMTV5bGVkOTljQ1FiYjhGTitONy9yQ3RmOVBk?=
 =?utf-8?B?dE1FY2FYNFFHTDM4TzQ0dzNaTStJY0V3ZjZnYld4SDhxdTA5SWJvZTUzUFNQ?=
 =?utf-8?B?empuYWpLTGhQZldDS3U5bWM1dXpMM2MxMUpCa3V4Y0dtT2crNUdxcGNPc0ti?=
 =?utf-8?B?NkcxTnZ1Q01iVEZ6QWo4ZWNKMGV4a1lkZXVXMEpFcWVDY2JYb0tBZXJVb2NK?=
 =?utf-8?B?Vk5wdjBDSlF1elJFemhvZmFnUTU5VjdlVjlHV0N4aWlvSEFZZjl5LzU2WWd5?=
 =?utf-8?B?bjd5TXZIQVkzaUtEWlFENXcwU1ZMSnFrSW4vNmd1UnZkRW9QOVg4Z0hEQTlC?=
 =?utf-8?B?UVRRbDVuc0cxRUtCd0ZNR0NNU0g0WHVNQUp6cG1pT1ZBZUcyUEtjVjMvcWlO?=
 =?utf-8?B?dExWZFNTeXhYZDBCNHNkSTJSdmRjdm5wdWJLVVBESFB5V04xKzMvTjE3T1pw?=
 =?utf-8?B?Y1hsRldobXVKY0JPd2lhZStMNGpDR3FTR2JYUUwzWmdBTGVJc2U3WlpZekNx?=
 =?utf-8?B?VzFuT0pKZjhiLytUZlAreVE0OCtwMEswb3d5emN5Q3ltU0pBUkltUElWdWVK?=
 =?utf-8?B?US9idUxJd29EczdFR1c0WmFvMzIrR3dIZmVFcVdaS2VRWTYzdVVHbmg2Tzho?=
 =?utf-8?B?NVg3dVRpRUM0WkhHeEhlNDgvMnVRVmhuWVNBL2ZDaVdZT1g0TEd1c1hJbDY2?=
 =?utf-8?B?R1ZCd2swSzIrOGEydWlvdXZNMm83Y1FVcTJxT1hvSFAvUUVlR0hWRENvNXY0?=
 =?utf-8?B?UjljWjI1Z3hFZ0Y3cUQ3YzZhcFZXeTJrelpPUFA1cXBQcXp6OWZvRmN3U004?=
 =?utf-8?B?Umg2ODRYdnc4T2dzVElsTnZncUlvYWNNekVMbjl0VERrRFZob0RpczRWdXRL?=
 =?utf-8?Q?8TB2ikTzaT8vx+pz82R48qK2X?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 59a7523b-0797-467e-e9fe-08da74514871
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Aug 2022 06:36:08.5600
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 11Dzg7ENTlf0AWYQH3CvLb8LeL/bPeQ6W3BYhRIrXh4lo8z0W2PBTTaXgXdfaw4Zgf21E/IKd6UTm8h8TAzY8Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1825
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dave,

On 7/22/2022 6:01 AM, Dave Chinner wrote:
> A huge amount of spinlock contention in the xlog_commit_cil() path
> went away. The commit identified doesn't remove/change any
> spinlocks, it actually adds more overhead to the critical section of
> the above spinlock in preparation for removing said spinlocks.
> 
> That removal happens in the next commit in that series - c0fb4765c508 ("xfs:
> convert CIL to unordered per cpu lists") - so I'd be expecting a
> bisect to demonstrate that the spinlock contention goes away with
> the commit that removed the spinlocks (as it does in all the testing
> of this I've done over the past 2 years), not the commit this bisect
> identified. Hence I think the bisect went wrong somewhere...

We did some investigation and got:

commit:
  df7a4a2134b0a ("xfs: convert CIL busy extents to per-cpu")
  016a23388cdcb ("xfs: Add order IDs to log items in CIL")
  c0fb4765c5086 ("xfs: convert CIL to unordered per cpu lists")

df7a4a2134b0a201 016a23388cdcb2740deb1379dc4 c0fb4765c5086cfd00f1158f5f4
---------------- --------------------------- ---------------------------
         %stddev     %change         %stddev     %change         %stddev
             \          |                \          |                \
     62.07            +0.0%      62.09            -0.0%      62.06        stress-ng.time.elapsed_time
     62.07            +0.0%      62.09            -0.0%      62.06        stress-ng.time.elapsed_time.max
      2237            +0.0%       2237            +0.0%       2237        stress-ng.time.file_system_inputs
      1842 ±  4%     +16.9%       2152 ±  3%     +17.6%       2166        stress-ng.time.involuntary_context_switches
    551.00            -0.3%     549.10            -0.3%     549.40        stress-ng.time.major_page_faults
      6376            -1.1%       6305 ±  2%      +0.6%       6416        stress-ng.time.maximum_resident_set_size
      9704            -0.3%       9676            -0.1%       9691        stress-ng.time.minor_page_faults
      4096            +0.0%       4096            +0.0%       4096        stress-ng.time.page_size
    841.90            -2.4%     821.70            -2.4%     821.90        stress-ng.time.percent_of_cpu_this_job_got
    512.83            -3.4%     495.24            -3.6%     494.18        stress-ng.time.system_time
     10.05 ±  8%     +52.3%      15.30 ±  3%     +61.1%      16.19 ±  2%  stress-ng.time.user_time
      2325 ± 16%     +66.5%       3873 ±  7%     +70.3%       3962 ±  6%  stress-ng.time.voluntary_context_switches
      1544 ±  4%     +54.4%       2385           +63.9%       2531        stress-ng.xattr.ops

Yes. commit c0fb4765c5086 ("xfs: convert CIL to unordered per cpu lists")
could bring performance gain also. But the most performance gain (54.4%)
is from commit 016a23388cdcb ("xfs: Add order IDs to log items in CIL").


Based on commit 016a23388cdcb and add following change:

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 6bc540898e3a..7c6c91a0a12d 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -659,9 +659,14 @@ xlog_cil_insert_items(
                        continue;
 
                lip->li_order_id = order;
-               if (!list_empty(&lip->li_cil))
-                       continue;
-               list_add_tail(&lip->li_cil, &cil->xc_cil);
+
+               /*
+                * Only move the item if it isn't already at the tail. This is
+                * to prevent a transient list_empty() state when reinserting
+                * an item that is already the only item in the CIL.
+                */
+               if (!list_is_last(&lip->li_cil, &cil->xc_cil))
+                       list_move_tail(&lip->li_cil, &cil->xc_cil);
        }

The performance will drop to the same level as commit df7a4a2134b0a
 ("xfs: convert CIL busy extents to per-cpu"):

commit: 
  016a23388cdcb2740deb1379dc408f21c84efb11
  a8bef09e7d8e65207c8403e030a0965db43ce3de

016a23388cdcb274 a8bef09e7d8e65207c8403e030a 
---------------- --------------------------- 
         %stddev     %change         %stddev
             \          |                \  
     62.06            -0.0%      62.05        stress-ng.time.elapsed_time
     62.06            -0.0%      62.05        stress-ng.time.elapsed_time.max
      2237            +0.0%       2237        stress-ng.time.file_system_inputs
      2226           -16.7%       1855 ±  4%  stress-ng.time.involuntary_context_switches
    549.00            +0.5%     551.67        stress-ng.time.major_page_faults
      6286            +0.1%       6292        stress-ng.time.maximum_resident_set_size
      9636            +0.1%       9641        stress-ng.time.minor_page_faults
      4096            +0.0%       4096        stress-ng.time.page_size
    823.00            +3.0%     847.33        stress-ng.time.percent_of_cpu_this_job_got
    496.02            +4.2%     516.61        stress-ng.time.system_time
     15.08           -38.1%       9.33 ±  4%  stress-ng.time.user_time
      4034 ±  3%     -43.0%       2299 ±  6%  stress-ng.time.voluntary_context_switches
      2368           -37.4%       1482 ±  4%  stress-ng.xattr.ops


Regards
Yin, Fengwei