| Message ID | 1479966490-8739-6-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Nov 24, 2016 at 04:48:08PM +1100, Alexey Kardashevskiy wrote: > We are going to allow the userspace to configure container in > one memory context and pass container fd to another so > we are postponing memory allocations accounted against > the locked memory limit. One of previous patches took care of > it_userspace. > > At the moment we create the default DMA window when the first group is > attached to a container; this is done for the userspace which is not > DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory > pre-registration - such client expects the default DMA window to exist. > > This postpones the default DMA window allocation till one of > the folliwing happens: > 1. first map/unmap request arrives; > 2. new window is requested; > This adds noop for the case when the userspace requested removal > of the default window which has not been created yet. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Hmm.. it just occurred to me: why do you even need to delay creation of the default window? In order to allow the open container - move to new process - map scenario you need to delay binding of the mm to the container until mapping actually occurs. And if you're doing that, you also want to delay allocation of the userspace table view, since the "userspace view" is only well defined once you're bound to a particular mm. But I don't see why you can't actually create the window - including the actual TCE table and all - before that point. After all, in the non-ddw case that's effectively what happens - the fixed window is there all the time. If you leave the creation of the default window at the point the first group is bound to the container, I think you'll simplify things - including because you'll remove an extra difference between the ddw and non-ddw cases. The you treat the binding of a table to an mm as a later step, that should go together with the allocation of the userspace view. > --- > Changes: > v6: > * new helper tce_iommu_create_default_window() moved to a separate patch; > * creates a default window when new window is requested; it used to > reset the def_window_pending flag instead; > * def_window_pending handling (mostly) localized in > tce_iommu_create_default_window() now, the only exception is removal > of not yet created default window. > --- > drivers/vfio/vfio_iommu_spapr_tce.c | 40 +++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > index a67bbfd..88622be 100644 > --- a/drivers/vfio/vfio_iommu_spapr_tce.c > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > @@ -97,6 +97,7 @@ struct tce_container { > struct mutex lock; > bool enabled; > bool v2; > + bool def_window_pending; > unsigned long locked_pages; > struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES]; > struct list_head group_list; > @@ -717,6 +718,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) > struct tce_iommu_group *tcegrp; > struct iommu_table_group *table_group; > > + if (!container->def_window_pending) > + return 0; > + > if (!tce_groups_attached(container)) > return -ENODEV; > > @@ -730,6 +734,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) > table_group->tce32_size, 1, &start_addr); > WARN_ON_ONCE(!ret && start_addr); > > + if (!ret) > + container->def_window_pending = false; > + > return ret; > } > > @@ -823,6 +830,10 @@ static long tce_iommu_ioctl(void *iommu_data, > VFIO_DMA_MAP_FLAG_WRITE)) > return -EINVAL; > > + ret = tce_iommu_create_default_window(container); > + if (ret) > + return ret; > + > num = tce_iommu_find_table(container, param.iova, &tbl); > if (num < 0) > return -ENXIO; > @@ -886,6 +897,10 @@ static long tce_iommu_ioctl(void *iommu_data, > if (param.flags) > return -EINVAL; > > + ret = tce_iommu_create_default_window(container); > + if (ret) > + return ret; > + > num = tce_iommu_find_table(container, param.iova, &tbl); > if (num < 0) > return -ENXIO; > @@ -1012,6 +1027,10 @@ static long tce_iommu_ioctl(void *iommu_data, > > mutex_lock(&container->lock); > > + ret = tce_iommu_create_default_window(container); > + if (ret) > + return ret; > + > ret = tce_iommu_create_window(container, create.page_shift, > create.window_size, create.levels, > &create.start_addr); > @@ -1044,6 +1063,11 @@ static long tce_iommu_ioctl(void *iommu_data, > if (remove.flags) > return -EINVAL; > > + if (container->def_window_pending && !remove.start_addr) { > + container->def_window_pending = false; > + return 0; > + } > + > mutex_lock(&container->lock); > > ret = tce_iommu_remove_window(container, remove.start_addr); > @@ -1141,7 +1165,6 @@ static int tce_iommu_attach_group(void *iommu_data, > struct tce_container *container = iommu_data; > struct iommu_table_group *table_group; > struct tce_iommu_group *tcegrp = NULL; > - bool create_default_window = false; > > mutex_lock(&container->lock); > > @@ -1189,25 +1212,12 @@ static int tce_iommu_attach_group(void *iommu_data, > } else { > ret = tce_iommu_take_ownership_ddw(container, table_group); > if (!tce_groups_attached(container) && !container->tables[0]) > - create_default_window = true; > + container->def_window_pending = true; > } > > if (!ret) { > tcegrp->grp = iommu_group; > list_add(&tcegrp->next, &container->group_list); > - /* > - * If it the first group attached, check if there is > - * a default DMA window and create one if none as > - * the userspace expects it to exist. > - */ > - if (create_default_window) { > - ret = tce_iommu_create_default_window(container); > - if (ret) { > - list_del(&tcegrp->next); > - tce_iommu_release_ownership_ddw(container, > - table_group); > - } > - } > } > > unlock_exit:
On 25/11/16 15:39, David Gibson wrote: > On Thu, Nov 24, 2016 at 04:48:08PM +1100, Alexey Kardashevskiy wrote: >> We are going to allow the userspace to configure container in >> one memory context and pass container fd to another so >> we are postponing memory allocations accounted against >> the locked memory limit. One of previous patches took care of >> it_userspace. >> >> At the moment we create the default DMA window when the first group is >> attached to a container; this is done for the userspace which is not >> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory >> pre-registration - such client expects the default DMA window to exist. >> >> This postpones the default DMA window allocation till one of >> the folliwing happens: >> 1. first map/unmap request arrives; >> 2. new window is requested; >> This adds noop for the case when the userspace requested removal >> of the default window which has not been created yet. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Hmm.. it just occurred to me: why do you even need to delay creation > of the default window? Because I want to account the memory it uses in locked_vm of the mm which will later be used for map/unmap. > > In order to allow the open container - move to new process - map > scenario you need to delay binding of the mm to the container until > mapping actually occurs. And if you're doing that, you also want to > delay allocation of the userspace table view, since the "userspace > view" is only well defined once you're bound to a particular mm. > > But I don't see why you can't actually create the window - including > the actual TCE table and all - before that point. After all, in the > non-ddw case that's effectively what happens - the fixed window is > there all the time. > > If you leave the creation of the default window at the point the first > group is bound to the container, I think you'll simplify things - > including because you'll remove an extra difference between the ddw > and non-ddw cases. > > The you treat the binding of a table to an mm as a later step, that > should go together with the allocation of the userspace view. > >> --- >> Changes: >> v6: >> * new helper tce_iommu_create_default_window() moved to a separate patch; >> * creates a default window when new window is requested; it used to >> reset the def_window_pending flag instead; >> * def_window_pending handling (mostly) localized in >> tce_iommu_create_default_window() now, the only exception is removal >> of not yet created default window. >> --- >> drivers/vfio/vfio_iommu_spapr_tce.c | 40 +++++++++++++++++++++++-------------- >> 1 file changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c >> index a67bbfd..88622be 100644 >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c >> @@ -97,6 +97,7 @@ struct tce_container { >> struct mutex lock; >> bool enabled; >> bool v2; >> + bool def_window_pending; >> unsigned long locked_pages; >> struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES]; >> struct list_head group_list; >> @@ -717,6 +718,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) >> struct tce_iommu_group *tcegrp; >> struct iommu_table_group *table_group; >> >> + if (!container->def_window_pending) >> + return 0; >> + >> if (!tce_groups_attached(container)) >> return -ENODEV; >> >> @@ -730,6 +734,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) >> table_group->tce32_size, 1, &start_addr); >> WARN_ON_ONCE(!ret && start_addr); >> >> + if (!ret) >> + container->def_window_pending = false; >> + >> return ret; >> } >> >> @@ -823,6 +830,10 @@ static long tce_iommu_ioctl(void *iommu_data, >> VFIO_DMA_MAP_FLAG_WRITE)) >> return -EINVAL; >> >> + ret = tce_iommu_create_default_window(container); >> + if (ret) >> + return ret; >> + >> num = tce_iommu_find_table(container, param.iova, &tbl); >> if (num < 0) >> return -ENXIO; >> @@ -886,6 +897,10 @@ static long tce_iommu_ioctl(void *iommu_data, >> if (param.flags) >> return -EINVAL; >> >> + ret = tce_iommu_create_default_window(container); >> + if (ret) >> + return ret; >> + >> num = tce_iommu_find_table(container, param.iova, &tbl); >> if (num < 0) >> return -ENXIO; >> @@ -1012,6 +1027,10 @@ static long tce_iommu_ioctl(void *iommu_data, >> >> mutex_lock(&container->lock); >> >> + ret = tce_iommu_create_default_window(container); >> + if (ret) >> + return ret; >> + >> ret = tce_iommu_create_window(container, create.page_shift, >> create.window_size, create.levels, >> &create.start_addr); >> @@ -1044,6 +1063,11 @@ static long tce_iommu_ioctl(void *iommu_data, >> if (remove.flags) >> return -EINVAL; >> >> + if (container->def_window_pending && !remove.start_addr) { >> + container->def_window_pending = false; >> + return 0; >> + } >> + >> mutex_lock(&container->lock); >> >> ret = tce_iommu_remove_window(container, remove.start_addr); >> @@ -1141,7 +1165,6 @@ static int tce_iommu_attach_group(void *iommu_data, >> struct tce_container *container = iommu_data; >> struct iommu_table_group *table_group; >> struct tce_iommu_group *tcegrp = NULL; >> - bool create_default_window = false; >> >> mutex_lock(&container->lock); >> >> @@ -1189,25 +1212,12 @@ static int tce_iommu_attach_group(void *iommu_data, >> } else { >> ret = tce_iommu_take_ownership_ddw(container, table_group); >> if (!tce_groups_attached(container) && !container->tables[0]) >> - create_default_window = true; >> + container->def_window_pending = true; >> } >> >> if (!ret) { >> tcegrp->grp = iommu_group; >> list_add(&tcegrp->next, &container->group_list); >> - /* >> - * If it the first group attached, check if there is >> - * a default DMA window and create one if none as >> - * the userspace expects it to exist. >> - */ >> - if (create_default_window) { >> - ret = tce_iommu_create_default_window(container); >> - if (ret) { >> - list_del(&tcegrp->next); >> - tce_iommu_release_ownership_ddw(container, >> - table_group); >> - } >> - } >> } >> >> unlock_exit: >
On Fri, Nov 25, 2016 at 05:38:26PM +1100, Alexey Kardashevskiy wrote: > On 25/11/16 15:39, David Gibson wrote: > > On Thu, Nov 24, 2016 at 04:48:08PM +1100, Alexey Kardashevskiy wrote: > >> We are going to allow the userspace to configure container in > >> one memory context and pass container fd to another so > >> we are postponing memory allocations accounted against > >> the locked memory limit. One of previous patches took care of > >> it_userspace. > >> > >> At the moment we create the default DMA window when the first group is > >> attached to a container; this is done for the userspace which is not > >> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory > >> pre-registration - such client expects the default DMA window to exist. > >> > >> This postpones the default DMA window allocation till one of > >> the folliwing happens: > >> 1. first map/unmap request arrives; > >> 2. new window is requested; > >> This adds noop for the case when the userspace requested removal > >> of the default window which has not been created yet. > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > Hmm.. it just occurred to me: why do you even need to delay creation > > of the default window? > > > Because I want to account the memory it uses in locked_vm of the mm which > will later be used for map/unmap. Ah, good point. How is the locked vm accounted for the non ddw case.
On 25/11/16 22:37, David Gibson wrote: > On Fri, Nov 25, 2016 at 05:38:26PM +1100, Alexey Kardashevskiy wrote: >> On 25/11/16 15:39, David Gibson wrote: >>> On Thu, Nov 24, 2016 at 04:48:08PM +1100, Alexey Kardashevskiy wrote: >>>> We are going to allow the userspace to configure container in >>>> one memory context and pass container fd to another so >>>> we are postponing memory allocations accounted against >>>> the locked memory limit. One of previous patches took care of >>>> it_userspace. >>>> >>>> At the moment we create the default DMA window when the first group is >>>> attached to a container; this is done for the userspace which is not >>>> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory >>>> pre-registration - such client expects the default DMA window to exist. >>>> >>>> This postpones the default DMA window allocation till one of >>>> the folliwing happens: >>>> 1. first map/unmap request arrives; >>>> 2. new window is requested; >>>> This adds noop for the case when the userspace requested removal >>>> of the default window which has not been created yet. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> >>> Hmm.. it just occurred to me: why do you even need to delay creation >>> of the default window? >> >> >> Because I want to account the memory it uses in locked_vm of the mm which >> will later be used for map/unmap. > > Ah, good point. How is the locked vm accounted for the non ddw case. When ioctl(ENABLE) is processed, the SPAPR TCE driver increments it. The DDW case does not use ENABLE, the memory preregistration is used instead.
On Thu, Nov 24, 2016 at 04:48:08PM +1100, Alexey Kardashevskiy wrote: > We are going to allow the userspace to configure container in > one memory context and pass container fd to another so > we are postponing memory allocations accounted against > the locked memory limit. One of previous patches took care of > it_userspace. > > At the moment we create the default DMA window when the first group is > attached to a container; this is done for the userspace which is not > DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory > pre-registration - such client expects the default DMA window to exist. > > This postpones the default DMA window allocation till one of > the folliwing happens: > 1. first map/unmap request arrives; > 2. new window is requested; > This adds noop for the case when the userspace requested removal > of the default window which has not been created yet. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > Changes: > v6: > * new helper tce_iommu_create_default_window() moved to a separate patch; > * creates a default window when new window is requested; it used to > reset the def_window_pending flag instead; > * def_window_pending handling (mostly) localized in > tce_iommu_create_default_window() now, the only exception is removal > of not yet created default window. > --- > drivers/vfio/vfio_iommu_spapr_tce.c | 40 +++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > index a67bbfd..88622be 100644 > --- a/drivers/vfio/vfio_iommu_spapr_tce.c > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > @@ -97,6 +97,7 @@ struct tce_container { > struct mutex lock; > bool enabled; > bool v2; > + bool def_window_pending; > unsigned long locked_pages; > struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES]; > struct list_head group_list; > @@ -717,6 +718,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) > struct tce_iommu_group *tcegrp; > struct iommu_table_group *table_group; > > + if (!container->def_window_pending) > + return 0; > + > if (!tce_groups_attached(container)) > return -ENODEV; > > @@ -730,6 +734,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) > table_group->tce32_size, 1, &start_addr); > WARN_ON_ONCE(!ret && start_addr); > > + if (!ret) > + container->def_window_pending = false; > + > return ret; > } > > @@ -823,6 +830,10 @@ static long tce_iommu_ioctl(void *iommu_data, > VFIO_DMA_MAP_FLAG_WRITE)) > return -EINVAL; > > + ret = tce_iommu_create_default_window(container); > + if (ret) > + return ret; > + > num = tce_iommu_find_table(container, param.iova, &tbl); > if (num < 0) > return -ENXIO; > @@ -886,6 +897,10 @@ static long tce_iommu_ioctl(void *iommu_data, > if (param.flags) > return -EINVAL; > > + ret = tce_iommu_create_default_window(container); > + if (ret) > + return ret; > + > num = tce_iommu_find_table(container, param.iova, &tbl); > if (num < 0) > return -ENXIO; > @@ -1012,6 +1027,10 @@ static long tce_iommu_ioctl(void *iommu_data, > > mutex_lock(&container->lock); > > + ret = tce_iommu_create_default_window(container); > + if (ret) > + return ret; > + > ret = tce_iommu_create_window(container, create.page_shift, > create.window_size, create.levels, > &create.start_addr); > @@ -1044,6 +1063,11 @@ static long tce_iommu_ioctl(void *iommu_data, > if (remove.flags) > return -EINVAL; > > + if (container->def_window_pending && !remove.start_addr) { > + container->def_window_pending = false; > + return 0; > + } > + > mutex_lock(&container->lock); > > ret = tce_iommu_remove_window(container, remove.start_addr); > @@ -1141,7 +1165,6 @@ static int tce_iommu_attach_group(void *iommu_data, > struct tce_container *container = iommu_data; > struct iommu_table_group *table_group; > struct tce_iommu_group *tcegrp = NULL; > - bool create_default_window = false; > > mutex_lock(&container->lock); > > @@ -1189,25 +1212,12 @@ static int tce_iommu_attach_group(void *iommu_data, > } else { > ret = tce_iommu_take_ownership_ddw(container, table_group); > if (!tce_groups_attached(container) && !container->tables[0]) > - create_default_window = true; > + container->def_window_pending = true; > } > > if (!ret) { > tcegrp->grp = iommu_group; > list_add(&tcegrp->next, &container->group_list); > - /* > - * If it the first group attached, check if there is > - * a default DMA window and create one if none as > - * the userspace expects it to exist. > - */ > - if (create_default_window) { > - ret = tce_iommu_create_default_window(container); > - if (ret) { > - list_del(&tcegrp->next); > - tce_iommu_release_ownership_ddw(container, > - table_group); > - } > - } > } > > unlock_exit:
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c index a67bbfd..88622be 100644 --- a/drivers/vfio/vfio_iommu_spapr_tce.c +++ b/drivers/vfio/vfio_iommu_spapr_tce.c @@ -97,6 +97,7 @@ struct tce_container { struct mutex lock; bool enabled; bool v2; + bool def_window_pending; unsigned long locked_pages; struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES]; struct list_head group_list; @@ -717,6 +718,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) struct tce_iommu_group *tcegrp; struct iommu_table_group *table_group; + if (!container->def_window_pending) + return 0; + if (!tce_groups_attached(container)) return -ENODEV; @@ -730,6 +734,9 @@ static long tce_iommu_create_default_window(struct tce_container *container) table_group->tce32_size, 1, &start_addr); WARN_ON_ONCE(!ret && start_addr); + if (!ret) + container->def_window_pending = false; + return ret; } @@ -823,6 +830,10 @@ static long tce_iommu_ioctl(void *iommu_data, VFIO_DMA_MAP_FLAG_WRITE)) return -EINVAL; + ret = tce_iommu_create_default_window(container); + if (ret) + return ret; + num = tce_iommu_find_table(container, param.iova, &tbl); if (num < 0) return -ENXIO; @@ -886,6 +897,10 @@ static long tce_iommu_ioctl(void *iommu_data, if (param.flags) return -EINVAL; + ret = tce_iommu_create_default_window(container); + if (ret) + return ret; + num = tce_iommu_find_table(container, param.iova, &tbl); if (num < 0) return -ENXIO; @@ -1012,6 +1027,10 @@ static long tce_iommu_ioctl(void *iommu_data, mutex_lock(&container->lock); + ret = tce_iommu_create_default_window(container); + if (ret) + return ret; + ret = tce_iommu_create_window(container, create.page_shift, create.window_size, create.levels, &create.start_addr); @@ -1044,6 +1063,11 @@ static long tce_iommu_ioctl(void *iommu_data, if (remove.flags) return -EINVAL; + if (container->def_window_pending && !remove.start_addr) { + container->def_window_pending = false; + return 0; + } + mutex_lock(&container->lock); ret = tce_iommu_remove_window(container, remove.start_addr); @@ -1141,7 +1165,6 @@ static int tce_iommu_attach_group(void *iommu_data, struct tce_container *container = iommu_data; struct iommu_table_group *table_group; struct tce_iommu_group *tcegrp = NULL; - bool create_default_window = false; mutex_lock(&container->lock); @@ -1189,25 +1212,12 @@ static int tce_iommu_attach_group(void *iommu_data, } else { ret = tce_iommu_take_ownership_ddw(container, table_group); if (!tce_groups_attached(container) && !container->tables[0]) - create_default_window = true; + container->def_window_pending = true; } if (!ret) { tcegrp->grp = iommu_group; list_add(&tcegrp->next, &container->group_list); - /* - * If it the first group attached, check if there is - * a default DMA window and create one if none as - * the userspace expects it to exist. - */ - if (create_default_window) { - ret = tce_iommu_create_default_window(container); - if (ret) { - list_del(&tcegrp->next); - tce_iommu_release_ownership_ddw(container, - table_group); - } - } } unlock_exit:
We are going to allow the userspace to configure container in one memory context and pass container fd to another so we are postponing memory allocations accounted against the locked memory limit. One of previous patches took care of it_userspace. At the moment we create the default DMA window when the first group is attached to a container; this is done for the userspace which is not DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory pre-registration - such client expects the default DMA window to exist. This postpones the default DMA window allocation till one of the folliwing happens: 1. first map/unmap request arrives; 2. new window is requested; This adds noop for the case when the userspace requested removal of the default window which has not been created yet. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v6: * new helper tce_iommu_create_default_window() moved to a separate patch; * creates a default window when new window is requested; it used to reset the def_window_pending flag instead; * def_window_pending handling (mostly) localized in tce_iommu_create_default_window() now, the only exception is removal of not yet created default window. --- drivers/vfio/vfio_iommu_spapr_tce.c | 40 +++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-)