[v2,08/12] iommu: Prepare for separating SVA and IOPF
Commit Message
Move iopf_group data structure to iommu.h. This is being done to make it
a minimal set of faults that a domain's page fault handler should handle.
Add two new helpers for the domain's page fault handler:
- iopf_free_group: free a fault group after all faults in the group are
handled.
- iopf_queue_work: queue a given work item for a fault group.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/iommu.h | 12 ++++++++++
drivers/iommu/io-pgfault.c | 48 ++++++++++++++++++++++----------------
2 files changed, 40 insertions(+), 20 deletions(-)
Comments
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> @@ -82,7 +82,7 @@ static void iopf_handler(struct work_struct *work)
> if (!domain || !domain->iopf_handler)
> status = IOMMU_PAGE_RESP_INVALID;
>
> - list_for_each_entry_safe(iopf, next, &group->faults, list) {
> + list_for_each_entry(iopf, &group->faults, list) {
> /*
> * For the moment, errors are sticky: don't handle
> subsequent
> * faults in the group if there is an error.
> @@ -90,14 +90,20 @@ static void iopf_handler(struct work_struct *work)
> if (status == IOMMU_PAGE_RESP_SUCCESS)
> status = domain->iopf_handler(&iopf->fault,
> domain->fault_data);
> -
> - if (!(iopf->fault.prm.flags &
> - IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
> - kfree(iopf);
> }
>
> iopf_complete_group(group->dev, &group->last_fault, status);
> - kfree(group);
> + iopf_free_group(group);
> +}
this is perf-critical path. It's not good to traverse the list twice.
> +
> +static int iopf_queue_work(struct iopf_group *group, work_func_t func)
> +{
> + struct iopf_device_param *iopf_param = group->dev->iommu-
> >iopf_param;
> +
> + INIT_WORK(&group->work, func);
> + queue_work(iopf_param->queue->wq, &group->work);
> +
> + return 0;
> }
Is there plan to introduce further error in the future? otherwise this should
be void.
btw the work queue is only for sva. If there is no other caller this can be
just kept in iommu-sva.c. No need to create a helper.
> @@ -199,8 +204,11 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> struct device *dev)
> list_move(&iopf->list, &group->faults);
> }
>
> - queue_work(iopf_param->queue->wq, &group->work);
> - return 0;
> + ret = iopf_queue_work(group, iopf_handler);
> + if (ret)
> + iopf_free_group(group);
> +
> + return ret;
>
Here we can document that the iopf handler (in patch10) should free the
group, allowing the optimization inside the handler.
On 2023/8/3 16:16, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> @@ -82,7 +82,7 @@ static void iopf_handler(struct work_struct *work)
>> if (!domain || !domain->iopf_handler)
>> status = IOMMU_PAGE_RESP_INVALID;
>>
>> - list_for_each_entry_safe(iopf, next, &group->faults, list) {
>> + list_for_each_entry(iopf, &group->faults, list) {
>> /*
>> * For the moment, errors are sticky: don't handle
>> subsequent
>> * faults in the group if there is an error.
>> @@ -90,14 +90,20 @@ static void iopf_handler(struct work_struct *work)
>> if (status == IOMMU_PAGE_RESP_SUCCESS)
>> status = domain->iopf_handler(&iopf->fault,
>> domain->fault_data);
>> -
>> - if (!(iopf->fault.prm.flags &
>> - IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
>> - kfree(iopf);
>> }
>>
>> iopf_complete_group(group->dev, &group->last_fault, status);
>> - kfree(group);
>> + iopf_free_group(group);
>> +}
>
> this is perf-critical path. It's not good to traverse the list twice.
Freeing the fault group is not critical anymore, right?
>
>> +
>> +static int iopf_queue_work(struct iopf_group *group, work_func_t func)
>> +{
>> + struct iopf_device_param *iopf_param = group->dev->iommu-
>>> iopf_param;
>> +
>> + INIT_WORK(&group->work, func);
>> + queue_work(iopf_param->queue->wq, &group->work);
>> +
>> + return 0;
>> }
>
> Is there plan to introduce further error in the future? otherwise this should
> be void.
queue_work() return true or false. I should check and return the value.
>
> btw the work queue is only for sva. If there is no other caller this can be
> just kept in iommu-sva.c. No need to create a helper.
The definition of struct iopf_device_param is in this file. So I added a
helper to avoid making iopf_device_param visible globally.
>
>> @@ -199,8 +204,11 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>> struct device *dev)
>> list_move(&iopf->list, &group->faults);
>> }
>>
>> - queue_work(iopf_param->queue->wq, &group->work);
>> - return 0;
>> + ret = iopf_queue_work(group, iopf_handler);
>> + if (ret)
>> + iopf_free_group(group);
>> +
>> + return ret;
>>
>
> Here we can document that the iopf handler (in patch10) should free the
> group, allowing the optimization inside the handler.
Yeah!
Best regards,
baolu
On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> Is there plan to introduce further error in the future? otherwise this should
> be void.
>
> btw the work queue is only for sva. If there is no other caller this can be
> just kept in iommu-sva.c. No need to create a helper.
I think more than just SVA will need a work queue context to process
their faults.
Jason
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, August 9, 2023 2:43 AM
>
> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>
> > Is there plan to introduce further error in the future? otherwise this should
> > be void.
> >
> > btw the work queue is only for sva. If there is no other caller this can be
> > just kept in iommu-sva.c. No need to create a helper.
>
> I think more than just SVA will need a work queue context to process
> their faults.
>
then this series needs more work. Currently the abstraction doesn't
include workqueue in the common fault reporting layer.
On 2023/8/9 8:02, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>> Sent: Wednesday, August 9, 2023 2:43 AM
>>
>> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>>
>>> Is there plan to introduce further error in the future? otherwise this should
>>> be void.
>>>
>>> btw the work queue is only for sva. If there is no other caller this can be
>>> just kept in iommu-sva.c. No need to create a helper.
>>
>> I think more than just SVA will need a work queue context to process
>> their faults.
>>
>
> then this series needs more work. Currently the abstraction doesn't
> include workqueue in the common fault reporting layer.
Do you mind elaborate a bit here? workqueue is a basic infrastructure in
the fault handling framework, but it lets the consumers choose to use
it, or not to.
Best regards,
baolu
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, August 9, 2023 6:41 PM
>
> On 2023/8/9 8:02, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@ziepe.ca>
> >> Sent: Wednesday, August 9, 2023 2:43 AM
> >>
> >> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> >>
> >>> Is there plan to introduce further error in the future? otherwise this
> should
> >>> be void.
> >>>
> >>> btw the work queue is only for sva. If there is no other caller this can be
> >>> just kept in iommu-sva.c. No need to create a helper.
> >>
> >> I think more than just SVA will need a work queue context to process
> >> their faults.
> >>
> >
> > then this series needs more work. Currently the abstraction doesn't
> > include workqueue in the common fault reporting layer.
>
> Do you mind elaborate a bit here? workqueue is a basic infrastructure in
> the fault handling framework, but it lets the consumers choose to use
> it, or not to.
>
My understanding of Jason's comment was to make the workqueue the
default path instead of being opted by the consumer.. that is my 1st
impression but might be wrong...
On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Wednesday, August 9, 2023 6:41 PM
> >
> > On 2023/8/9 8:02, Tian, Kevin wrote:
> > >> From: Jason Gunthorpe <jgg@ziepe.ca>
> > >> Sent: Wednesday, August 9, 2023 2:43 AM
> > >>
> > >> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> > >>
> > >>> Is there plan to introduce further error in the future? otherwise this
> > should
> > >>> be void.
> > >>>
> > >>> btw the work queue is only for sva. If there is no other caller this can be
> > >>> just kept in iommu-sva.c. No need to create a helper.
> > >>
> > >> I think more than just SVA will need a work queue context to process
> > >> their faults.
> > >>
> > >
> > > then this series needs more work. Currently the abstraction doesn't
> > > include workqueue in the common fault reporting layer.
> >
> > Do you mind elaborate a bit here? workqueue is a basic infrastructure in
> > the fault handling framework, but it lets the consumers choose to use
> > it, or not to.
> >
>
> My understanding of Jason's comment was to make the workqueue the
> default path instead of being opted by the consumer.. that is my 1st
> impression but might be wrong...
Yeah, that is one path. Do we have anyone that uses this that doesn't
want the WQ? (actually who even uses this besides SVA?)
Jason
On 2023/8/11 0:47, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>> Sent: Wednesday, August 9, 2023 6:41 PM
>>>
>>> On 2023/8/9 8:02, Tian, Kevin wrote:
>>>>> From: Jason Gunthorpe<jgg@ziepe.ca>
>>>>> Sent: Wednesday, August 9, 2023 2:43 AM
>>>>>
>>>>> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>>>>>
>>>>>> Is there plan to introduce further error in the future? otherwise this
>>> should
>>>>>> be void.
>>>>>>
>>>>>> btw the work queue is only for sva. If there is no other caller this can be
>>>>>> just kept in iommu-sva.c. No need to create a helper.
>>>>> I think more than just SVA will need a work queue context to process
>>>>> their faults.
>>>>>
>>>> then this series needs more work. Currently the abstraction doesn't
>>>> include workqueue in the common fault reporting layer.
>>> Do you mind elaborate a bit here? workqueue is a basic infrastructure in
>>> the fault handling framework, but it lets the consumers choose to use
>>> it, or not to.
>>>
>> My understanding of Jason's comment was to make the workqueue the
>> default path instead of being opted by the consumer.. that is my 1st
>> impression but might be wrong...
> Yeah, that is one path. Do we have anyone that uses this that doesn't
> want the WQ? (actually who even uses this besides SVA?)
I am still confused. When we forward iopf's to user space through the
iommufd, we don't need to schedule a WQ, right? Or I misunderstood here?
Best regards,
baolu
On Fri, Aug 11, 2023 at 09:53:41AM +0800, Baolu Lu wrote:
> On 2023/8/11 0:47, Jason Gunthorpe wrote:
> > On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
> > > > From: Baolu Lu<baolu.lu@linux.intel.com>
> > > > Sent: Wednesday, August 9, 2023 6:41 PM
> > > >
> > > > On 2023/8/9 8:02, Tian, Kevin wrote:
> > > > > > From: Jason Gunthorpe<jgg@ziepe.ca>
> > > > > > Sent: Wednesday, August 9, 2023 2:43 AM
> > > > > >
> > > > > > On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> > > > > >
> > > > > > > Is there plan to introduce further error in the future? otherwise this
> > > > should
> > > > > > > be void.
> > > > > > >
> > > > > > > btw the work queue is only for sva. If there is no other caller this can be
> > > > > > > just kept in iommu-sva.c. No need to create a helper.
> > > > > > I think more than just SVA will need a work queue context to process
> > > > > > their faults.
> > > > > >
> > > > > then this series needs more work. Currently the abstraction doesn't
> > > > > include workqueue in the common fault reporting layer.
> > > > Do you mind elaborate a bit here? workqueue is a basic infrastructure in
> > > > the fault handling framework, but it lets the consumers choose to use
> > > > it, or not to.
> > > >
> > > My understanding of Jason's comment was to make the workqueue the
> > > default path instead of being opted by the consumer.. that is my 1st
> > > impression but might be wrong...
> > Yeah, that is one path. Do we have anyone that uses this that doesn't
> > want the WQ? (actually who even uses this besides SVA?)
>
> I am still confused. When we forward iopf's to user space through the
> iommufd, we don't need to schedule a WQ, right? Or I misunderstood
> here?
Yes, that could be true, iommufd could just queue it from the
interrupt context and trigger a wakeup.
But other iommufd modes would want to invoke hmm_range_fault() which
would need the work queue.
Jason
On 2023/8/11 21:27, Jason Gunthorpe wrote:
> On Fri, Aug 11, 2023 at 09:53:41AM +0800, Baolu Lu wrote:
>> On 2023/8/11 0:47, Jason Gunthorpe wrote:
>>> On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
>>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>>> Sent: Wednesday, August 9, 2023 6:41 PM
>>>>>
>>>>> On 2023/8/9 8:02, Tian, Kevin wrote:
>>>>>>> From: Jason Gunthorpe<jgg@ziepe.ca>
>>>>>>> Sent: Wednesday, August 9, 2023 2:43 AM
>>>>>>>
>>>>>>> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>>>>>>>
>>>>>>>> Is there plan to introduce further error in the future? otherwise this
>>>>> should
>>>>>>>> be void.
>>>>>>>>
>>>>>>>> btw the work queue is only for sva. If there is no other caller this can be
>>>>>>>> just kept in iommu-sva.c. No need to create a helper.
>>>>>>> I think more than just SVA will need a work queue context to process
>>>>>>> their faults.
>>>>>>>
>>>>>> then this series needs more work. Currently the abstraction doesn't
>>>>>> include workqueue in the common fault reporting layer.
>>>>> Do you mind elaborate a bit here? workqueue is a basic infrastructure in
>>>>> the fault handling framework, but it lets the consumers choose to use
>>>>> it, or not to.
>>>>>
>>>> My understanding of Jason's comment was to make the workqueue the
>>>> default path instead of being opted by the consumer.. that is my 1st
>>>> impression but might be wrong...
>>> Yeah, that is one path. Do we have anyone that uses this that doesn't
>>> want the WQ? (actually who even uses this besides SVA?)
>> I am still confused. When we forward iopf's to user space through the
>> iommufd, we don't need to schedule a WQ, right? Or I misunderstood
>> here?
> Yes, that could be true, iommufd could just queue it from the
> interrupt context and trigger a wakeup.
>
> But other iommufd modes would want to invoke hmm_range_fault() which
> would need the work queue.
Yes. That's the reason why I added below helper
int iopf_queue_work(struct iopf_group *group, work_func_t func)
in the patch 09/12.
Best regards,
baolu
@@ -503,6 +503,18 @@ struct dev_iommu {
u32 pci_32bit_workaround:1;
};
+struct iopf_fault {
+ struct iommu_fault fault;
+ struct list_head list;
+};
+
+struct iopf_group {
+ struct iopf_fault last_fault;
+ struct list_head faults;
+ struct work_struct work;
+ struct device *dev;
+};
+
int iommu_device_register(struct iommu_device *iommu,
const struct iommu_ops *ops,
struct device *hwdev);
@@ -40,17 +40,17 @@ struct iopf_device_param {
struct list_head partial;
};
-struct iopf_fault {
- struct iommu_fault fault;
- struct list_head list;
-};
+static void iopf_free_group(struct iopf_group *group)
+{
+ struct iopf_fault *iopf, *next;
-struct iopf_group {
- struct iopf_fault last_fault;
- struct list_head faults;
- struct work_struct work;
- struct device *dev;
-};
+ list_for_each_entry_safe(iopf, next, &group->faults, list) {
+ if (!(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
+ kfree(iopf);
+ }
+
+ kfree(group);
+}
static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
enum iommu_page_response_code status)
@@ -71,9 +71,9 @@ static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
static void iopf_handler(struct work_struct *work)
{
+ struct iopf_fault *iopf;
struct iopf_group *group;
struct iommu_domain *domain;
- struct iopf_fault *iopf, *next;
enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
group = container_of(work, struct iopf_group, work);
@@ -82,7 +82,7 @@ static void iopf_handler(struct work_struct *work)
if (!domain || !domain->iopf_handler)
status = IOMMU_PAGE_RESP_INVALID;
- list_for_each_entry_safe(iopf, next, &group->faults, list) {
+ list_for_each_entry(iopf, &group->faults, list) {
/*
* For the moment, errors are sticky: don't handle subsequent
* faults in the group if there is an error.
@@ -90,14 +90,20 @@ static void iopf_handler(struct work_struct *work)
if (status == IOMMU_PAGE_RESP_SUCCESS)
status = domain->iopf_handler(&iopf->fault,
domain->fault_data);
-
- if (!(iopf->fault.prm.flags &
- IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
- kfree(iopf);
}
iopf_complete_group(group->dev, &group->last_fault, status);
- kfree(group);
+ iopf_free_group(group);
+}
+
+static int iopf_queue_work(struct iopf_group *group, work_func_t func)
+{
+ struct iopf_device_param *iopf_param = group->dev->iommu->iopf_param;
+
+ INIT_WORK(&group->work, func);
+ queue_work(iopf_param->queue->wq, &group->work);
+
+ return 0;
}
/**
@@ -190,7 +196,6 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
group->last_fault.fault = *fault;
INIT_LIST_HEAD(&group->faults);
list_add(&group->last_fault.list, &group->faults);
- INIT_WORK(&group->work, iopf_handler);
/* See if we have partial faults for this group */
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
@@ -199,8 +204,11 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
list_move(&iopf->list, &group->faults);
}
- queue_work(iopf_param->queue->wq, &group->work);
- return 0;
+ ret = iopf_queue_work(group, iopf_handler);
+ if (ret)
+ iopf_free_group(group);
+
+ return ret;
cleanup_partial:
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {