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: <bf8b9a97-e499-55e3-be91-12fa3a76687c@oracle.com>
Date:   Wed, 11 May 2022 10:34:13 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH] net/vdpa: Use reader/writers semaphore instead of mutex
Content-Language: en-US
To:     Eli Cohen <elic@nvidia.com>, mst@redhat.com, jasowang@redhat.com
Cc:     virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
References: <20220511125845.1332586-1-elic@nvidia.com>
From:   Si-Wei Liu <si-wei.liu@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20220511125845.1332586-1-elic@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TDZ1VG1EZXk1aCtjNFhLbzMvNFBUazY3N0puSy8zSnpVdDZGenhwTmN2enU5?=
 =?utf-8?B?WVNHRjJ3emZYYThSdjlwTUVrelk0U3hRYkZtcEF4cGgyYUZ1ZnY5V0JLWjY1?=
 =?utf-8?B?YjUyTy9uOUV5dWk5ZitSUnQyTlNjZjBwZWJEMGNMMmlLV05rM0lSR3Rvcm9P?=
 =?utf-8?B?bS9ZcFJnL0tiOG51WjB0ZWNvdjgveStORFlGS3d3MFo4bmdiaXhTcWNtSHI0?=
 =?utf-8?B?VlFwMU1TdmVQZHREMXpyc1MrOEZRblp0MjdEMDErVWpNOURnc013TGZtM0Zo?=
 =?utf-8?B?VVE3cFhvL05uSjVjaFROMHNyNkE2cWJUZHhOL0Q4aytCMmErVjE1WkRlOVZw?=
 =?utf-8?B?OGlWOXNtZjd6bHBUSExFMTF4RFFWTHBlVmgwYVRxU29ud0dlb1F0MzJxYVdR?=
 =?utf-8?B?dGpaa3VmVTkreTREOFR2OTArd0dlTnl0RXRVZDEzYUlpSlQyT2V2dG15RGFQ?=
 =?utf-8?B?NnppdUkvd2lOekZnci9vR2xYc1JoNUZJcnZiQW5wSGVSMFNKZkhMUlFGcEFz?=
 =?utf-8?B?bktUVVpYMVQxeW5Zb0xsWTFXQjdJeFdCNFYrd0tCTHNCbDVsOXNJRHVvSWZs?=
 =?utf-8?B?NVlTVEFwdVNZdm8yOENzdi93RWMyOXhsUkxBbFVFU3g0T0JGTGoyWUplMUpm?=
 =?utf-8?B?dkVzVWplSW5weVdmdFZsMHU2dm5pNEh2bXR5Y1pSNDljRzVyamlQUnJmeGV6?=
 =?utf-8?B?RXpBcG45UTFxVXFkTWhFbExhR2d0ek9vdTAvbDFxQ096dm9hS0JlS1Ixamh0?=
 =?utf-8?B?VmQzMUloS1VSSkcxbHhSTHo1ekxtVmk5bnd3cFJwYUE0bW9WNUxlN3cvU0Y0?=
 =?utf-8?B?UlNwNEI1elNGaVNYOW84ak90dTFDL0pCd2c2SVZWekpEWWp3M3ZBczk0NzRV?=
 =?utf-8?B?WHZVQmsxa1lQNGpkTktMRDErbmZZYm5kMlplYjZFRnk2R0tLc2cwakpTUjRi?=
 =?utf-8?B?R2NNSXBBaFFUd3YxZTBmVSt5THRjUlVMZmVuaWczSUltMTRKZ3Y5cDdCbE9H?=
 =?utf-8?B?NmZVZXUwYTY1SE5wZjNlV1VwSk45eDJzUUZ5OFp6VjdJTzFpeVd1N24zRW1m?=
 =?utf-8?B?c3Q1cnZ0cFRjeWJjakdiRlJlYVA0THhtUUYyWWVWbk9PM0ErT3hQWDRISkI3?=
 =?utf-8?B?OHdNWEoyOEc5a0FmMGNya1FGU2NZSzduWTBVRVNsZ0dDOEpOazVaUlpnZmFa?=
 =?utf-8?B?c2ZEbXFoREROZjVjd2tzY0g3cTVhOHdiaUpIeWU2cXNva1craE82dm9GNGxt?=
 =?utf-8?B?a0RSMTVxY25YQ2hwMHQrNmE5MStxVyt5MVh6WEM4WVVIbkhrU1J1dkNFa1RV?=
 =?utf-8?B?NU4zditJQkpHUlhpU0RMd1VoOGR0UGtXT1pEZ1dQWTVyR0h0KzZra2xiWkRI?=
 =?utf-8?B?bzBaa1Bmci9kbC9yczVSaUNYQ01ic2t4S29EdnltVnZjaG9xc3BLcXExcm5D?=
 =?utf-8?B?dGlWMGhRM0lmbDhPQUJoTDRvUEJFdmRPcmYwM0JtNmhZREdibzF0YTU3TGI1?=
 =?utf-8?B?MHhrbjJFZXlmS2FDZnNGUzZXdXJIdVNBenlJZ2FHZm5Xak1pbnhNZnNudU5E?=
 =?utf-8?B?ZWp5eGptQjBOZCtJOTdtMmJzOUxsem1jcjI0TThmdkpoeEg3eUpmQk9vWFVH?=
 =?utf-8?B?Z2I1TEQzaTR4K0gyZG9jNENOOU1ZSEpiWXVJeFFNM3lkRlFnTEliSzh0TC9X?=
 =?utf-8?B?RHFYRjgvalZrclJPakRKUzlkVW9sTDU5U3hlN3hPVmRVVTd2eGxOYWVNU0sz?=
 =?utf-8?B?Nkl0L0hJaHJ2QUdhRjZYaXZBakpqTFJxcEpuelpsd1psWmlHck4vTVFnNjRE?=
 =?utf-8?B?TFFmdWFIdnhhM1hsTzF0ZnM3ZmF6d0V3YlpkeG9LaDdPVmhJUmhqWGlzREIr?=
 =?utf-8?B?eUZDNzZuR0txNDZiTmx3dWlNaEtGT1J3NVFnZGl3RWh2S2VCd2tYb2NndkF4?=
 =?utf-8?B?REQ1UTY4RUl4Um5NanY0czdYR2ZTT2Z2akZTNmpmUHIrdHRHVWhkRzBpbDJh?=
 =?utf-8?B?SGdlTHpRL20vYVZBT21XUVJKMmdaYUlQR3NrRGlvdGYvdFc5MDdmOVRNamdw?=
 =?utf-8?B?WXhlZURMOHpONXFVUzBpSThyZkYrMWdYMzd1NjN4dlFCUHRFRGdVMGJLWHZI?=
 =?utf-8?B?Rk9MZWFZMks1YjdtMlowZ2xnYWV5a0lSM0ZoSzBBOW94amtlZG56Z3ZCVmEx?=
 =?utf-8?B?LzVJYzVHVlErUXdZSUM1YVdSU2hYeFNKNS9salhiOGFJM3Jjb016MmlpZ0ps?=
 =?utf-8?B?VVV4MG5NRGZSQWppQzJ4ZXNxVGNNQk5aTitoRTZodDZsVXEvZ0JhV055K1Zp?=
 =?utf-8?B?YUZydjNNbFpxQ3Z4ZXdEd3crUTFsWE5NNFQzRzRTNHc0SVVrTGpHZzdYMmY4?=
 =?utf-8?Q?9p0e+OKXSx9f9R1c=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 21fe32fc-6d84-4036-6e54-08da3374796b
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3287.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 May 2022 17:34:17.5621
 (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: t+AAz3g3T3FVrZ4a4lzsYYx+OzW/+etMVgNZGQUfD4HVdzAGfRR1oXo4uEdvL689Md6ZLR0smXg+82jLD72kSw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR10MB2676
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-11_07:2022-05-11,2022-05-11 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 suspectscore=0 phishscore=0 spamscore=0 bulkscore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205110079
X-Proofpoint-ORIG-GUID: zgAkksweUUdKcZUkdroj-EzzE2BlUs7L
X-Proofpoint-GUID: zgAkksweUUdKcZUkdroj-EzzE2BlUs7L
X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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 5/11/2022 5:58 AM, Eli Cohen wrote:
> Use rw_semaphore instead of mutex to control access to vdpa devices.
> This can be especially beneficial in case process poll on statistics
> information.
>
> Suggested-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
LGTM. It might be good to pack this patch into the vendor stat series 
such that the newly added vdpa_dev_vendor_stats_fill() may use 
rw_semaphore on top.

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>

> ---
>   drivers/vdpa/vdpa.c | 64 ++++++++++++++++++++++-----------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 0fb4a615f267..ced634f94f76 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -18,7 +18,7 @@
>   
>   static LIST_HEAD(mdev_head);
>   /* A global mutex that protects vdpa management device and device level operations. */
> -static DEFINE_MUTEX(vdpa_dev_mutex);
> +static DECLARE_RWSEM(vdpa_dev_lock);
>   static DEFINE_IDA(vdpa_index_ida);
>   
>   void vdpa_set_status(struct vdpa_device *vdev, u8 status)
> @@ -238,7 +238,7 @@ static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
>   
>   	vdev->nvqs = nvqs;
>   
> -	lockdep_assert_held(&vdpa_dev_mutex);
> +	lockdep_assert_held(&vdpa_dev_lock);
>   	dev = bus_find_device(&vdpa_bus, NULL, dev_name(&vdev->dev), vdpa_name_match);
>   	if (dev) {
>   		put_device(dev);
> @@ -278,9 +278,9 @@ int vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
>   {
>   	int err;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_write(&vdpa_dev_lock);
>   	err = __vdpa_register_device(vdev, nvqs);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_write(&vdpa_dev_lock);
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(vdpa_register_device);
> @@ -293,7 +293,7 @@ EXPORT_SYMBOL_GPL(vdpa_register_device);
>    */
>   void _vdpa_unregister_device(struct vdpa_device *vdev)
>   {
> -	lockdep_assert_held(&vdpa_dev_mutex);
> +	lockdep_assert_held(&vdpa_dev_lock);
>   	WARN_ON(!vdev->mdev);
>   	device_unregister(&vdev->dev);
>   }
> @@ -305,9 +305,9 @@ EXPORT_SYMBOL_GPL(_vdpa_unregister_device);
>    */
>   void vdpa_unregister_device(struct vdpa_device *vdev)
>   {
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_write(&vdpa_dev_lock);
>   	device_unregister(&vdev->dev);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_write(&vdpa_dev_lock);
>   }
>   EXPORT_SYMBOL_GPL(vdpa_unregister_device);
>   
> @@ -352,9 +352,9 @@ int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev)
>   		return -EINVAL;
>   
>   	INIT_LIST_HEAD(&mdev->list);
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_write(&vdpa_dev_lock);
>   	list_add_tail(&mdev->list, &mdev_head);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_write(&vdpa_dev_lock);
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(vdpa_mgmtdev_register);
> @@ -371,14 +371,14 @@ static int vdpa_match_remove(struct device *dev, void *data)
>   
>   void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev)
>   {
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_write(&vdpa_dev_lock);
>   
>   	list_del(&mdev->list);
>   
>   	/* Filter out all the entries belong to this management device and delete it. */
>   	bus_for_each_dev(&vdpa_bus, NULL, mdev, vdpa_match_remove);
>   
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_write(&vdpa_dev_lock);
>   }
>   EXPORT_SYMBOL_GPL(vdpa_mgmtdev_unregister);
>   
> @@ -532,17 +532,17 @@ static int vdpa_nl_cmd_mgmtdev_get_doit(struct sk_buff *skb, struct genl_info *i
>   	if (!msg)
>   		return -ENOMEM;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	mdev = vdpa_mgmtdev_get_from_attr(info->attrs);
>   	if (IS_ERR(mdev)) {
> -		mutex_unlock(&vdpa_dev_mutex);
> +		up_read(&vdpa_dev_lock);
>   		NL_SET_ERR_MSG_MOD(info->extack, "Fail to find the specified mgmt device");
>   		err = PTR_ERR(mdev);
>   		goto out;
>   	}
>   
>   	err = vdpa_mgmtdev_fill(mdev, msg, info->snd_portid, info->snd_seq, 0);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	if (err)
>   		goto out;
>   	err = genlmsg_reply(msg, info);
> @@ -561,7 +561,7 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>   	int idx = 0;
>   	int err;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	list_for_each_entry(mdev, &mdev_head, list) {
>   		if (idx < start) {
>   			idx++;
> @@ -574,7 +574,7 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>   		idx++;
>   	}
>   out:
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	cb->args[0] = idx;
>   	return msg->len;
>   }
> @@ -630,7 +630,7 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>   	    !netlink_capable(skb, CAP_NET_ADMIN))
>   		return -EPERM;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_write(&vdpa_dev_lock);
>   	mdev = vdpa_mgmtdev_get_from_attr(info->attrs);
>   	if (IS_ERR(mdev)) {
>   		NL_SET_ERR_MSG_MOD(info->extack, "Fail to find the specified management device");
> @@ -650,7 +650,7 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>   
>   	err = mdev->ops->dev_add(mdev, name, &config);
>   err:
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_write(&vdpa_dev_lock);
>   	return err;
>   }
>   
> @@ -666,7 +666,7 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>   		return -EINVAL;
>   	name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_write(&vdpa_dev_lock);
>   	dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
>   	if (!dev) {
>   		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> @@ -684,7 +684,7 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>   mdev_err:
>   	put_device(dev);
>   dev_err:
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_write(&vdpa_dev_lock);
>   	return err;
>   }
>   
> @@ -750,7 +750,7 @@ static int vdpa_nl_cmd_dev_get_doit(struct sk_buff *skb, struct genl_info *info)
>   	if (!msg)
>   		return -ENOMEM;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>   	if (!dev) {
>   		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> @@ -768,13 +768,13 @@ static int vdpa_nl_cmd_dev_get_doit(struct sk_buff *skb, struct genl_info *info)
>   
>   	err = genlmsg_reply(msg, info);
>   	put_device(dev);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	return err;
>   
>   mdev_err:
>   	put_device(dev);
>   err:
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	nlmsg_free(msg);
>   	return err;
>   }
> @@ -816,9 +816,9 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
>   	info.start_idx = cb->args[0];
>   	info.idx = 0;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	bus_for_each_dev(&vdpa_bus, NULL, &info, vdpa_dev_dump);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	cb->args[0] = info.idx;
>   	return msg->len;
>   }
> @@ -1016,7 +1016,7 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
>   	if (!msg)
>   		return -ENOMEM;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>   	if (!dev) {
>   		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> @@ -1037,7 +1037,7 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
>   mdev_err:
>   	put_device(dev);
>   dev_err:
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	if (err)
>   		nlmsg_free(msg);
>   	return err;
> @@ -1075,9 +1075,9 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>   	info.start_idx = cb->args[0];
>   	info.idx = 0;
>   
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	bus_for_each_dev(&vdpa_bus, NULL, &info, vdpa_dev_config_dump);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	cb->args[0] = info.idx;
>   	return msg->len;
>   }
> @@ -1104,7 +1104,7 @@ static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>   		return -ENOMEM;
>   
>   	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> -	mutex_lock(&vdpa_dev_mutex);
> +	down_read(&vdpa_dev_lock);
>   	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>   	if (!dev) {
>   		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> @@ -1124,7 +1124,7 @@ static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>   	err = genlmsg_reply(msg, info);
>   
>   	put_device(dev);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   
>   	return err;
>   
> @@ -1132,7 +1132,7 @@ static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>   	put_device(dev);
>   dev_err:
>   	nlmsg_free(msg);
> -	mutex_unlock(&vdpa_dev_mutex);
> +	up_read(&vdpa_dev_lock);
>   	return err;
>   }
>