Received-SPF: pass (google.com: domain of linux-kernel+bounces-211419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <45603be5-65ae-4d45-bfbe-22c3c1e22280@oracle.com>
Date: Wed, 12 Jun 2024 12:33:19 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 12/27] blk-iocost: grab ioc->lock for debt handling
To: Tejun Heo <tj@kernel.org>, axboe@kernel.dk
Cc: linux-block@vger.kernel.org, cgroups@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com, newella@fb.com,
        hch@lst.de
References: <20200901185257.645114-1-tj@kernel.org>
 <20200901185257.645114-13-tj@kernel.org>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20200901185257.645114-13-tj@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?ME5XWEpzOUVjelIzVXJadzg2SlFtOTM5WVp4cEVFbHJnZDhEOVU1NktEZHFY?=
 =?utf-8?B?dkZmTGY4Z2QyMmlyc3djVDBqbWVlZ3k3VXU3VkhOSm5sSTJ4MUQ4bU16NFVU?=
 =?utf-8?B?bWQ4RWdkcHdGNDQzYTU0UjdHWDl3eHpvZVZxRzVWQWlPS0V5N2NEai9LREJ0?=
 =?utf-8?B?NlhBY3JuQXd4WjhpWGNLTG04L2pGdWhUSVptbS9mdkM3cENJWGFwZkQ1SU8v?=
 =?utf-8?B?dGZ4d0N3OSsxVERxVnRWOEsvNGYwdW1VMytDazhXSkg0NnZYV0pNM25wSERH?=
 =?utf-8?B?RmMzYUxES00wVk1jTkN1UzRlaTF5WWp6clhDSDBoYTJyYXNVUU5ZQTQraVNK?=
 =?utf-8?B?aDFKa251M3lkSEJwQzYraWJuUWJrOGJCemR6dDVFUkRzUzZDUEdSVkNRVlJM?=
 =?utf-8?B?dTV4WCsrcEMxcFI2VjhJeW9yUHl1MnNOSXo2N2hIaUdkU1BoNHZNanozU3VL?=
 =?utf-8?B?OTg1WDNFQmNYRmoxSVB2dVIzUU4yb0owRUtmN003REk1VGxJMC9SWk1EYU9M?=
 =?utf-8?B?TVpsc0Q0NS9nYTFuV3l3Nkxxbm5EY0NHdFlkVlIwNE5zNEFlaW96aUNWTmlx?=
 =?utf-8?B?S2tseXFPTzRJZ2FjTndQSWs2Vi9qcHo5bVNSd09yY1pWdTFBaGEwWFkzdEIw?=
 =?utf-8?B?UXpKMDBHN2g3MmN2Q1ZWdEdVR2Z6Szg3d1VDNklsZ2VVRjN2NVdhWGJNUHEw?=
 =?utf-8?B?NzFWMkIrZE9NYy8zZEhGRngxcXJOZ2x6N0gwUjM4dHpLa3MyUFNTYUFXYzJP?=
 =?utf-8?B?Yit1V01nWDVpdjZrRXI3a1dNRmxlRGd2NlpPSm42WmpsUmxSUEVjeHp5SHRV?=
 =?utf-8?B?bDRoY2JkT2Y0OG0rNjFFTDFZbE01V0VkQUVRY2tLaUduU0ZYMkJGL1hYcEc0?=
 =?utf-8?B?bWVqcGl4U09EdWJ3NmNPT2RqekdpTHR2NTkyb1RHaXk5RXAvUHZmdDJPcnJi?=
 =?utf-8?B?alJNeUtIc0NrUmQ1WklheE1QVHoxczkyQjU4YkNraEdlejZZanJ0N3JpbnJS?=
 =?utf-8?B?a0NkVlRZNEJva0c5RFhRTEFhbFBDaW5laUdIRy9FZkp0ZmViTXRRUy9iNDh4?=
 =?utf-8?B?UThBbFFlcXhiWWxRekUwSVVtQVU5WTBPSmRpd3BhZzJta0JDYlJ3RkN5RzRD?=
 =?utf-8?B?QUM0UFBsWDQvMVdraTdSZnIvaHhVV1hUUkJFaFd0aXlpWVJ1akhKK0srakNv?=
 =?utf-8?B?L0tHMTg3a2U1WHVvVW9GRGRmMmFzRGJaYTZvZENuVE1kYjZQZW9JbjBmdXYw?=
 =?utf-8?B?NThkTGdscjRNQm8yNGdZUHFIRVFkTUNad0lyWG9aN3hjc0tYbkpqOE02am9V?=
 =?utf-8?B?ZEw2MjA5QXdjcUhPS2VLdTF6b243b1A2WjJXMjhRdXg4RWhBN1Nyc0QyZGY4?=
 =?utf-8?B?aXVEZzhydVpMclpqdDIvNFlxUkZuSjRIVjBVUVB1eWJNV3p4RkFYNmpXWDQw?=
 =?utf-8?B?WXdwdmo5c3NVWjY0ZVhBeXBqalJRZytPbFFiRmJkUkZtN2hjVU9oTWlVclBX?=
 =?utf-8?B?YmdhUEQzR0dHcFBvN3FZVHluZDlEQk9DeXU1NTQ1S2d1TnNHL25EL3lPeFFP?=
 =?utf-8?B?YVZuOGV6VnVwbjNybExNcERGbmtEWUlrWXZabVQzSEEvcG5LdnUxQmV5NktL?=
 =?utf-8?B?ZmI3UVBiTFdBaFFCMVk3TlJPMVJlZzFJb0dldkluSlhwN0lwYUJXUFlzUFZj?=
 =?utf-8?B?R2RNVis3K0pzamN0L3drcDJIOFpHV0JCZVE1KzhjNzJsaFJndCtKYzVvMERQ?=
 =?utf-8?B?QnNoUjZUNFBxcUtHbGlUcUFBMnV4QTdmRnFHQzJ3QlFPR25hcFVmRjJmb1RS?=
 =?utf-8?B?Ymc1enlrYzM0MnJQZmltMFd0NktEUUkzMys1aVB2blRRNllCeWhmbGo4Wk9r?=
 =?utf-8?B?SlphWUhKSFVwVzl1SEswWUQ5MDNLeFNZc0xKUUVPcXJpT0wrOUNaNUtZWEM4?=
 =?utf-8?B?VnhITzNXa1VSZzg1WjdKUWRncExKU1Z0K05NU3RacG02QjJWYmsyT2llZzI5?=
 =?utf-8?B?d2srK083akR3QnNlS2VIVlB3ZGE1Rmt3K2xETnMzNGZOQkJNV2YyZUpsd0t4?=
 =?utf-8?B?RTJaSzhDZzJzL0p1S1VrZEJqTktUaVBMT2w4bm9kaVNhR3I3RFVtdUk5RUZB?=
 =?utf-8?B?MWxJcjQzOHE3Q3ZiZWFJLzRkR0RpM3Q3QXIxbjNnZFRITkY3RUFXWjk4Wkdz?=
 =?utf-8?B?V1E9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	2eiT7rMfeGxshfFMpsBU/JO3KDmXdv/H99RAyyVXonsfJNBc3ENojWrFnVmjod6MF65CDCK7BQqZVD+kAG/Xdlxf1WKp9bnmWu+hVdhXJB8NqzpIflPgqkZ9SRDr+7TLurJDy+VOl17eflW/MQ5hKaYW2siNXPcwl9/UdsoaOnmPabkasLJLhN85lRHDxTNVN/fnvhyVUZpcrzZynPnp+d6UnXqku17E8iufF9/KATqe90Osy43rUnc+hZ7CBKxpzE7Q3izZ8HsxP8S2xbk9Xly8fN8yxRb2tAq47FWDmHTsTC9Q6vHb8i2t58DlUOI0Ss/VcgL3Kw/vR2JXM7BEAZcWQCzvrpLfJjNrde//E2/hxd3yqr3gvFwmQNuonV9CKn+Ml3AjNxxAda7MZWAThRGV4rJplmyUNbgeQcVQ+vGAxZSCKYhRMJq2H8yx/LGOs6dsBWTa8H/q9M562CebItfwTubxRRaXigTTyP8AT1SJFGZ34pO1htKP+fS8z026Ud/c52ZEwtx/lsASthmmk5I/A7MiVJefiywLXtcmlt8bH/Y6St6ug8zkA3piWHgJ00TdMgE1qh7xmC/RazyadfRgLAyHAdhQmIFDFQIMJ+s=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f64ae51e-9a6b-47dc-e2f8-08dc8ad3782a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jun 2024 11:33:24.0734
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8Zqax6CJx6mCfpPoKJ8hV/th3CY5V1I92hiXnJCNaZ6lm4dYds8R3yW21rXaJy8y8KKmAZhTPbn1dq+JaHmejw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR10MB5932
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-12_06,2024-06-12_02,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 phishscore=0 suspectscore=0 malwarescore=0 mlxscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406120084
X-Proofpoint-GUID: QletfVoY3h_oTn-nZBmJ7K8elbkwNtWE
X-Proofpoint-ORIG-GUID: QletfVoY3h_oTn-nZBmJ7K8elbkwNtWE

On 01/09/2020 19:52, Tejun Heo wrote:

Background:

I have been trying to solve some block/ sparse issues, and it has led me 
to digging up this old mail.

> Currently, debt handling requires only iocg->waitq.lock. In the future, we
> want to adjust and propagate inuse changes depending on debt status. Let's
> grab ioc->lock in debt handling paths in preparation.
> 
> * Because ioc->lock nests outside iocg->waitq.lock, the decision to grab
>    ioc->lock needs to be made before entering the critical sections.
> 
> * Add and use iocg_[un]lock() which handles the conditional double locking.
> 
> * Add @pay_debt to iocg_kick_waitq() so that debt payment happens only when
>    the caller grabbed both locks.
> 
> This patch is prepatory and the comments contain references to future
> changes.
> 
> Signed-off-by: Tejun Heo<tj@kernel.org>
> ---
>   block/blk-iocost.c | 92 ++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 73 insertions(+), 19 deletions(-)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index f36988657594..23b173e34591 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -680,6 +680,26 @@ static void iocg_commit_bio(struct ioc_gq *iocg, struct bio *bio, u64 cost)
>   	atomic64_add(cost, &iocg->vtime);
>   }
>   
> +static void iocg_lock(struct ioc_gq *iocg, bool lock_ioc, unsigned long *flags)
> +{
> +	if (lock_ioc) {
> +		spin_lock_irqsave(&iocg->ioc->lock, *flags);
> +		spin_lock(&iocg->waitq.lock);
> +	} else {
> +		spin_lock_irqsave(&iocg->waitq.lock, *flags);
> +	}
> +}

This generates the following sparse warnings on mainline today:

   CHECK   block/blk-iocost.c
block/blk-iocost.c:685:9: warning: context imbalance in 'iocg_lock' -
wrong count at exit
block/blk-iocost.c:696:28: warning: context imbalance in 'iocg_unlock'
- unexpected unlock

If we try to break iocg_lock() into one version for lock_ioc set and 
another for lock_ioc unset, we can solve the sparse issues for those 
functions, but then we get another sparse issue from the callsites for 
those functions:

block/blk-iocost.c:2679:17: warning: context imbalance in
'ioc_rqos_throttle' - different lock contexts for basic block

I tried to solve with a total ioc_rqos_throttle() re-org and much code 
duplication by calling the different lock and unlock versions from 
effectively 2x separate copies of ioc_rqos_throttle(), as sparse seems 
confused with how we call these functions. It's a total no-go.

Any simpler idea to solve these? Or just something to live with?

Thanks,
John

> +
> +static void iocg_unlock(struct ioc_gq *iocg, bool unlock_ioc, unsigned long *flags)
> +{
> +	if (unlock_ioc) {
> +		spin_unlock(&iocg->waitq.lock);
> +		spin_unlock_irqrestore(&iocg->ioc->lock, *flags);
> +	} else {
> +		spin_unlock_irqrestore(&iocg->waitq.lock, *flags);
> +	}
> +}