Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH] vfio/mtty: Enforce available_instances
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <cohuck@redhat.com>, <jgg@nvidia.com>
References: <162465624894.3338367.12935940647049917981.stgit@omen>
 <ee949a98-6998-2032-eb17-00ef8b8d911c@nvidia.com>
 <20210628125602.5b07388e.alex.williamson@redhat.com>
From:   Kirti Wankhede <kwankhede@nvidia.com>
Message-ID: <641a865f-a45b-10ed-8287-3759191a9686@nvidia.com>
Date:   Tue, 29 Jun 2021 01:22:00 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210628125602.5b07388e.alex.williamson@redhat.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jun 2021 19:52:06.8456
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: a65ff0a6-836c-4b21-30a6-08d93a6e3598
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT014.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5248
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/29/2021 12:26 AM, Alex Williamson wrote:
> On Mon, 28 Jun 2021 23:19:54 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 6/26/2021 2:56 AM, Alex Williamson wrote:
>>> The sample mtty mdev driver doesn't actually enforce the number of
>>> device instances it claims are available.  Implement this properly.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>
>>> Applies to vfio next branch + Jason's atomic conversion
>>>    
>>
>>
>> Does this need to be on top of Jason's patch?
> 
> Yes, see immediately above.
> 
>> Patch to use mdev_used_ports is reverted here, can it be changed from
>> mdev_devices_list to mdev_avail_ports atomic variable?
> 
> It doesn't revert Jason's change, it builds on it.  The patches could
> we squashed, but there's no bug in Jason's patch that we're trying to
> avoid exposing, so I don't see why we'd do that.
>

'Squashed' is the correct word that 'revert', my bad.

>> Change here to use atomic variable looks good to me.
>>
>> Reviewed by: Kirti Wankhede <kwankhede@nvidia.com>
> 
> Thanks!  It was Jason's patch[1] that converted to use an atomic
> though, so I'm slightly confused if this R-b is for the patch below,
> Jason's patch, or both.  Thanks,

I liked 'mdev_avail_ports' approach than 'mdev_used_ports' approach 
here. This R-b is for below patch.

Thanks,
Kirti

> 
> Alex
> 
> [1]https://lore.kernel.org/kvm/0-v1-0bc56b362ca7+62-mtty_used_ports_jgg@nvidia.com/
> 
>>>    samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
>>>    1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
>>> index ffbaf07a17ea..8b26fecc4afe 100644
>>> --- a/samples/vfio-mdev/mtty.c
>>> +++ b/samples/vfio-mdev/mtty.c
>>> @@ -144,7 +144,7 @@ struct mdev_state {
>>>    	int nr_ports;
>>>    };
>>>    
>>> -static atomic_t mdev_used_ports;
>>> +static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
>>>    
>>>    static const struct file_operations vd_fops = {
>>>    	.owner          = THIS_MODULE,
>>> @@ -707,11 +707,20 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    {
>>>    	struct mdev_state *mdev_state;
>>>    	int nr_ports = mdev_get_type_group_id(mdev) + 1;
>>> +	int avail_ports = atomic_read(&mdev_avail_ports);
>>>    	int ret;
>>>    
>>> +	do {
>>> +		if (avail_ports < nr_ports)
>>> +			return -ENOSPC;
>>> +	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
>>> +				     &avail_ports, avail_ports - nr_ports));
>>> +
>>>    	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
>>> -	if (mdev_state == NULL)
>>> +	if (mdev_state == NULL) {
>>> +		atomic_add(nr_ports, &mdev_avail_ports);
>>>    		return -ENOMEM;
>>> +	}
>>>    
>>>    	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
>>>    
>>> @@ -724,6 +733,7 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    
>>>    	if (mdev_state->vconfig == NULL) {
>>>    		kfree(mdev_state);
>>> +		atomic_add(nr_ports, &mdev_avail_ports);
>>>    		return -ENOMEM;
>>>    	}
>>>    
>>> @@ -735,9 +745,9 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    	ret = vfio_register_group_dev(&mdev_state->vdev);
>>>    	if (ret) {
>>>    		kfree(mdev_state);
>>> +		atomic_add(nr_ports, &mdev_avail_ports);
>>>    		return ret;
>>>    	}
>>> -	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
>>>    
>>>    	dev_set_drvdata(&mdev->dev, mdev_state);
>>>    	return 0;
>>> @@ -746,12 +756,13 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    static void mtty_remove(struct mdev_device *mdev)
>>>    {
>>>    	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
>>> +	int nr_ports = mdev_state->nr_ports;
>>>    
>>> -	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
>>>    	vfio_unregister_group_dev(&mdev_state->vdev);
>>>    
>>>    	kfree(mdev_state->vconfig);
>>>    	kfree(mdev_state);
>>> +	atomic_add(nr_ports, &mdev_avail_ports);
>>>    }
>>>    
>>>    static int mtty_reset(struct mdev_state *mdev_state)
>>> @@ -1271,8 +1282,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>>>    {
>>>    	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
>>>    
>>> -	return sprintf(buf, "%d\n",
>>> -		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
>>> +	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
>>>    }
>>>    
>>>    static MDEV_TYPE_ATTR_RO(available_instances);
>>>
>>>    
>>
>