On 1/3/2023 1:13 PM, Steven Sistare wrote:
> On 1/3/2023 10:21 AM, Jason Gunthorpe wrote:
>> On Tue, Dec 20, 2022 at 12:39:21PM -0800, Steve Sistare wrote:
>>> Track locked_vm per dma struct, and create a new subroutine, both for use
>>> in a subsequent patch. No functional change.
>>>
>>> Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
>>> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>>> ---
>>> drivers/vfio/vfio_iommu_type1.c | 20 +++++++++++++++-----
>>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>> index 71f980b..588d690 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -101,6 +101,7 @@ struct vfio_dma {
>>> struct rb_root pfn_list; /* Ex-user pinned pfn list */
>>> unsigned long *bitmap;
>>> struct mm_struct *mm;
>>> + long locked_vm;
>>
>> Why is it long? Can it be negative?
>
> The existing code uses both long and uint64_t for page counts, and I picked one.
> I'll use size_t instead to match vfio_dma size.
>
>>> };
>>>
>>> struct vfio_batch {
>>> @@ -413,22 +414,21 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn)
>>> return ret;
>>> }
>>>
>>> -static int vfio_lock_acct(struct vfio_dma *dma, long npage, bool async)
>>> +static int mm_lock_acct(struct task_struct *task, struct mm_struct *mm,
>>> + bool lock_cap, long npage, bool async)
>>> {
>>
>> Now async is even more confusing, the caller really should have a
>> valid handle on the mm before using it as an argument like this.
>
> The caller holds a grab reference on mm, and mm_lock_acct does mmget_not_zero to
> validate the mm. IMO this is a close analog of the original vfio_lock_acct code
> where the caller holds a get reference on task, and does get_task_mm to validate
> the mm.
>
> However, I can hoist the mmget_not_zero from mm_lock_acct to its callsites in
> vfio_lock_acct and vfio_change_dma_owner.
Yielding:
static int mm_lock_acct(struct task_struct *task, struct mm_struct *mm,
bool lock_cap, long npage)
{
int ret = mmap_write_lock_killable(mm);
if (!ret) {
ret = __account_locked_vm(mm, abs(npage), npage > 0, task,
lock_cap);
mmap_write_unlock(mm);
}
return ret;
}
static int vfio_lock_acct(struct vfio_dma *dma, long npage, bool async)
{
struct mm_struct *mm = dma->mm;
int ret;
if (!npage)
return 0;
if (async && !mmget_not_zero(mm))
return -ESRCH; /* process exited */
ret = mm_lock_acct(dma->task, mm, dma->lock_cap, npage);
if (!ret)
dma->locked_vm += npage;
if (async)
mmput(mm);
return ret;
}
static int vfio_change_dma_owner(struct vfio_dma *dma)
{
...
ret = mm_lock_acct(task, mm, lock_cap, npage);
if (ret)
return ret;
if (mmget_not_zero(dma->mm)) {
mm_lock_acct(dma->task, dma->mm, dma->lock_cap, -npage);
mmput(dma->mm);
}
...
- Steve
