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;
Received-SPF: Pass (protection.outlook.com: domain of seco.com designates
 20.160.56.86 as permitted sender) receiver=protection.outlook.com;
 client-ip=20.160.56.86; helo=inpost-eu.tmcas.trendmicro.com; pr=C
Authentication-Results-Original: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=seco.com;
Message-ID: <e9c2b2df-72e5-f0ab-728a-255e8b2e565d@seco.com>
Date:   Tue, 18 Apr 2023 11:22:50 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH v3 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock
Content-Language: en-US
To:     Crystal Wood <oss@buserror.net>, Li Yang <leoyang.li@nxp.com>,
        linuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org
Cc:     Vladimir Oltean <vladimir.oltean@nxp.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Camelia Groza <camelia.groza@nxp.com>,
        linux-kernel@vger.kernel.org, Roy Pledge <roy.pledge@nxp.com>,
        "David S . Miller" <davem@davemloft.net>,
        Madalin Bucur <madalin.bucur@nxp.com>
References: <20230404145557.2356894-1-sean.anderson@seco.com>
 <20230404145557.2356894-2-sean.anderson@seco.com>
 <48dacc58c7c04ba8a005d8edd56744c8455f007e.camel@buserror.net>
 <d4737c45-2bbf-d364-9768-20baa46f6af4@seco.com>
 <3b707d1c-1120-274f-6cd6-b3283a334563@seco.com>
 <497c92b50103a4ba3469cd41edbd967ee9bfb291.camel@buserror.net>
From:   Sean Anderson <sean.anderson@seco.com>
In-Reply-To: <497c92b50103a4ba3469cd41edbd967ee9bfb291.camel@buserror.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-Microsoft-Antispam-Message-Info-Original: 1Wr6hZqCtiCN2v95JDDW/2jvIccgo4m90ocBk4xxyiXXwYinT88Gov7cCTQUaLEwlWg2ur+2mMnwnPgpCoCa6+wE2oBu3KJQtnCXscTv2m3iK6S3N9DllPI9Hig6YscZEjWWS7Gz4OfW36sZ2UhoHK1SQ8a4ndPcPbJQiQXhvdpxEpkY4FCjEdG14G59CMEU8O2kaYnk7tHkPo+si97+pJJtv8v/nT8UjyatfR3BhaL6QmVbzVfDq3r8+4ChsAtw7ccoZRjgqbeeMdbQJNv2LqunTwhONYkPpBwbYvYoC5bf0bTXEQ0+IaKd5NsKXRjY4QRALXyGmKSI6+HFIPlbQvjIWY5fabCY2wOTRF52I2QfxtBXtVEE5Hh3Y3N7OSzDiYxIm+6WCQu1cSWSyqv+ilswtxH2hr4KLTb5GfUyBxWldXBXQh5EwB1s71X9r0D9W+lCSStwsxdqoYxdweWshRpMepc0wMkMsN4epUWcK1lNQp8TxmJyKq2DXJDtZGKBfoVg8E/cM2jIclTpDHquuWW8cxwFMkpvpKMwI9n2Ao5KW7l/xCFUe0jR0/G57eiq+9ZD9KtBSk8CMPgg4CxvDMonGAgRvaAeAmDaW+7LwSl9A3bYUZfxIjVQrjX4MoBRea9sWjDF9SwCKGGANv16cuTmzd2+jyCbVBWNgRQVCZAm4EM1mDHYAg2wXd93r1a8
X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DB9PR03MB8847.eurprd03.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(366004)(346002)(136003)(39850400004)(376002)(396003)(451199021)(6506007)(86362001)(31696002)(26005)(83380400001)(6512007)(53546011)(36756003)(186003)(2906002)(44832011)(7416002)(31686004)(2616005)(6486002)(52116002)(5660300002)(38350700002)(38100700002)(6666004)(8676002)(8936002)(66476007)(478600001)(110136005)(54906003)(41300700001)(316002)(66556008)(66946007)(4326008)(43740500002)(45980500001);DIR:OUT;SFP:1101;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAWPR03MB9788
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped: AM6EUR05FT036.eop-eur05.prod.protection.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs: 6eadbde8-6ea6-460a-16ec-08db4020c8d7
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: pfjckRU0hiSSlcw0wDNgOxyOT57yO27Ry4PepPYhDnQCM84aerifCG+Ztk7LwTIqnD16wR5xMchpP7p6Hs0Jk/Cf5b/kMIGC2SB9uOrA/NoFq/fQzKx1Zdj3k8Ym1Nn/atHMCf/GllVx7+M8J1MKaNz5E+p3IFFGT5rxATIeyEYB6Wxky9cdyc7k04z/cGpzcdKPoIUcnR0Cd8XISk1eBETysiFPAYYU9kMQgw+pEPlyUHDfulNoj7DbgVq4qEqBUgzCoP2VdWziEUsvxuCT7tVUzGR45pNdMK3KGZVdMGVv9soLU17In7+d4zyMOftgUpMUsko7MwPjqUUG/HIgp6ZXWpMIpJVEK/vByLvshLch8jwRq25M9PJwN0i4yjhFgL74G4KZrsGYXPC6JsmxKGn3I67uVhF0fyOVgHiEozFFtElkyBXKH2hEjvfyvyctrPwnM9eYcJdyW5YqAd+ELP8ZkzTU/EqgT1teiKD1V4CSC2Hv4XDmKc0rnQ85wK7gqldxUvTsnXCUD88O8v5x+5GAcXmbVldsrpte6PQLkgyhE23QI+akGjx3AyL5CLXe3xGpsPnqlq2Bah1G7cJY8MXped0Am3oZWAjXl5pFISiJF+jYmfQFt+tJTlGLtEp30qKdVhCuIklhjhLw1vOoDdQu46AW1EW0AG8uUzdxs3bg5/zvp8fmijVOtlMaO25Pp9oLfHKvs48+FcszQmJtD8lHuHArksLZIssW16RXgN1I3I2BtdnxJIzorHq+OheDqokeUiyhU2V0/7kN09hEYA==
X-Forefront-Antispam-Report: CIP:20.160.56.86;CTRY:NL;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:inpost-eu.tmcas.trendmicro.com;PTR:inpost-eu.tmcas.trendmicro.com;CAT:NONE;SFS:(13230028)(396003)(346002)(136003)(39850400004)(376002)(451199021)(46966006)(40470700004)(36840700001)(36860700001)(2616005)(110136005)(6512007)(6506007)(26005)(6666004)(40480700001)(86362001)(54906003)(34070700002)(478600001)(31686004)(316002)(4326008)(83380400001)(336012)(186003)(6486002)(53546011)(47076005)(70206006)(70586007)(7596003)(356005)(7636003)(82740400003)(5660300002)(44832011)(7416002)(41300700001)(2906002)(40460700003)(8936002)(8676002)(31696002)(36756003)(82310400005)(43740500002);DIR:OUT;SFP:1101;
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2023 15:23:03.8874
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 268a86b5-78a6-4f95-9595-08db4020cdd0
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=bebe97c3-6438-442e-ade3-ff17aa50e733;Ip=[20.160.56.86];Helo=[inpost-eu.tmcas.trendmicro.com]
X-MS-Exchange-CrossTenant-AuthSource: AM6EUR05FT036.eop-eur05.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR03MB9566
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED 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

On 4/18/23 02:29, Crystal Wood wrote:
> On Tue, 2023-04-11 at 11:09 -0400, Sean Anderson wrote:
>> Hi Crystal,
>> 
>> On 4/4/23 12:04, Sean Anderson wrote:
>> > On 4/4/23 11:33, Crystal Wood wrote:
>> > > On Tue, 2023-04-04 at 10:55 -0400, Sean Anderson wrote:
>> > > 
>> > > > @@ -1456,11 +1456,11 @@ static void tqm_congestion_task(struct
>> > > > work_struct
>> > > > *work)
>> > > >         union qm_mc_result *mcr;
>> > > >         struct qman_cgr *cgr;
>> > > >  
>> > > > -       spin_lock_irq(&p->cgr_lock);
>> > > > +       raw_spin_lock_irq(&p->cgr_lock);
>> > > >         qm_mc_start(&p->p);
>> > > >         qm_mc_commit(&p->p, QM_MCC_VERB_QUERYCONGESTION);
>> > > >         if (!qm_mc_result_timeout(&p->p, &mcr)) {
>> > > > -               spin_unlock_irq(&p->cgr_lock);
>> > > > +               raw_spin_unlock_irq(&p->cgr_lock);
>> > > 
>> > > qm_mc_result_timeout() spins with a timeout of 10 ms which is very
>> > > inappropriate for a raw lock.  What is the actual expected upper bound?
>> > 
>> > Hm, maybe we can move this qm_mc stuff outside cgr_lock? In most other
>> > places they're called without cgr_lock, which implies that its usage
>> > here is meant to synchronize against some other function.
>> 
>> Do you have any suggestions here? I think this should really be handled
>> in a follow-up patch. If you think this code is waiting too long in a raw
>> spinlock, the existing code can wait just as long with IRQs disabled.
>> This patch doesn't change existing system responsiveness.
> 
> Well, AFAICT it expands the situations in which it happens from configuration
> codepaths to stuff like congestion handling.  The proper fix would probably be
> to use some mechanism other than smp_call_function_single() to run code on the
> target cpu so that it can run with irqs enabled (or get confirmation that the
> actual worst case is short enough),

Well, we used to use a kthread/wait_for_completion before 96f413f47677
("soc/fsl/qbman: fix issue in qman_delete_cgr_safe()"). The commit
description there isn't very enlightening as to the actual bug, which is
why I CC's Madalin earlier. To be honest, I'm not really familiar with
the other options in the kernel.

> but barring that I guess at least acknowledge the situation in a
> comment?

Fine by me.

--Sean