| Message ID | d1ca6ac5-0ed4-200f-c4e0-7a657b8d8fa8@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86/mm: don't needlessly veto migration | expand |
On 08/10/2019 17:10, Jan Beulich wrote: > From: Paul Durrant <paul.durrant@citrix.com> > > Now that xl.cfg has an option to explicitly enable IOMMU mappings for a > domain, migration may be needlessly vetoed due to the check of > is_iommu_enabled() in paging_log_dirty_enable(). > There is actually no need to prevent logdirty from being enabled unless > devices are assigned to a domain. > > NOTE: While in the neighbourhood, the bool_t parameter type in > paging_log_dirty_enable() is replaced with a bool and the format > of the comment in assign_device() is fixed. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Release-acked-by: Juergen Gross <jgross@suse.com> Seriously FFS. Why am I having to repeat myself? What if any way unclear on the previous threads? NACK NACK NACK. Xen is, and has always been, the wrong place to have any logic, because IT DOESN'T HAVE ENOUGH INFORMATION TO MAKE THE DECISION CORRECTLY. The toolstack does. Therefore, the toolstack is the only level capable decide whether it is safe to migration/suspend/resume/checkpoint the VM. If I have to write the patches myself, I will, but this patch in this form is frankly unacceptable. ~Andrew
On 08.10.2019 18:38, Andrew Cooper wrote: > On 08/10/2019 17:10, Jan Beulich wrote: >> From: Paul Durrant <paul.durrant@citrix.com> >> >> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a >> domain, migration may be needlessly vetoed due to the check of >> is_iommu_enabled() in paging_log_dirty_enable(). >> There is actually no need to prevent logdirty from being enabled unless >> devices are assigned to a domain. >> >> NOTE: While in the neighbourhood, the bool_t parameter type in >> paging_log_dirty_enable() is replaced with a bool and the format >> of the comment in assign_device() is fixed. >> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Release-acked-by: Juergen Gross <jgross@suse.com> > > Seriously FFS. Why am I having to repeat myself? What if any way > unclear on the previous threads? > > NACK NACK NACK. Xen is, and has always been, the wrong place to have > any logic, because IT DOESN'T HAVE ENOUGH INFORMATION TO MAKE THE > DECISION CORRECTLY. > > The toolstack does. > > Therefore, the toolstack is the only level capable decide whether it is > safe to migration/suspend/resume/checkpoint the VM. > > If I have to write the patches myself, I will, but this patch in this > form is frankly unacceptable. You're kidding, aren't you? By taking only part of Paul's original patch, we should be able to get rid of two of the current osstest reported regressions. At the same time this _does not_ exclude an incremental subsequent patch to also add the other half (see my reply to him yesterday suggesting this split). The two steps shouldn't have been merged into a single patch anyway imo: The part here fixes a regression, while the other part changes original behavior, and continues to be (irrespective of your wording, which once again suggests that in certain cases you aren't willing to tolerate thinking that's different from yours) controversial. If it helps, I can change the title (and perhaps description) to make it look less like the original patch, and to put focus on the regression. I just didn't want to massage it more than absolutely needed. Jan
On Wed, 9 Oct 2019 at 09:49, Jan Beulich <jbeulich@suse.com> wrote: > > On 08.10.2019 18:38, Andrew Cooper wrote: > > On 08/10/2019 17:10, Jan Beulich wrote: > >> From: Paul Durrant <paul.durrant@citrix.com> > >> > >> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a > >> domain, migration may be needlessly vetoed due to the check of > >> is_iommu_enabled() in paging_log_dirty_enable(). > >> There is actually no need to prevent logdirty from being enabled unless > >> devices are assigned to a domain. > >> > >> NOTE: While in the neighbourhood, the bool_t parameter type in > >> paging_log_dirty_enable() is replaced with a bool and the format > >> of the comment in assign_device() is fixed. > >> > >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> Release-acked-by: Juergen Gross <jgross@suse.com> > > > > Seriously FFS. Why am I having to repeat myself? What if any way > > unclear on the previous threads? > > > > NACK NACK NACK. Xen is, and has always been, the wrong place to have > > any logic, because IT DOESN'T HAVE ENOUGH INFORMATION TO MAKE THE > > DECISION CORRECTLY. > > > > The toolstack does. > > > > Therefore, the toolstack is the only level capable decide whether it is > > safe to migration/suspend/resume/checkpoint the VM. > > > > If I have to write the patches myself, I will, but this patch in this > > form is frankly unacceptable. > > You're kidding, aren't you? By taking only part of Paul's original > patch, we should be able to get rid of two of the current osstest > reported regressions. At the same time this _does not_ exclude an > incremental subsequent patch to also add the other half (see my > reply to him yesterday suggesting this split). The two steps > shouldn't have been merged into a single patch anyway imo: The > part here fixes a regression, while the other part changes original > behavior, and continues to be (irrespective of your wording, which > once again suggests that in certain cases you aren't willing to > tolerate thinking that's different from yours) controversial. > > If it helps, I can change the title (and perhaps description) to > make it look less like the original patch, and to put focus on the > regression. I just didn't want to massage it more than absolutely > needed. Agreed. Given where we are w.r.t. regressions and a release schedule, I think we need to be pragmatic. Realistically I'm not going get a Xen dev. environment up and running for maybe a week so I can't work on this myself at the moment. I am happy for Jan to fix the regressions and then we can move on after 4.13 is out the door. Paul > > Jan
On 10/8/19 5:10 PM, Jan Beulich wrote: > From: Paul Durrant <paul.durrant@citrix.com> > > Now that xl.cfg has an option to explicitly enable IOMMU mappings for a > domain, migration may be needlessly vetoed due to the check of > is_iommu_enabled() in paging_log_dirty_enable(). > There is actually no need to prevent logdirty from being enabled unless > devices are assigned to a domain. > > NOTE: While in the neighbourhood, the bool_t parameter type in > paging_log_dirty_enable() is replaced with a bool and the format > of the comment in assign_device() is fixed. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Release-acked-by: Juergen Gross <jgross@suse.com> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
On 08.10.2019 18:38, Andrew Cooper wrote: > On 08/10/2019 17:10, Jan Beulich wrote: >> From: Paul Durrant <paul.durrant@citrix.com> >> >> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a >> domain, migration may be needlessly vetoed due to the check of >> is_iommu_enabled() in paging_log_dirty_enable(). >> There is actually no need to prevent logdirty from being enabled unless >> devices are assigned to a domain. >> >> NOTE: While in the neighbourhood, the bool_t parameter type in >> paging_log_dirty_enable() is replaced with a bool and the format >> of the comment in assign_device() is fixed. >> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Release-acked-by: Juergen Gross <jgross@suse.com> > > Seriously FFS. Why am I having to repeat myself? What if any way > unclear on the previous threads? > > NACK NACK NACK. With George having given his R-b I'm now in an awkward position: I shouldn't ignore this triple NACK and commit the patch, but there's also no sensible way forward here which would allow the regression to be taken care of without committing the patch in its current shape. Are you willing to take back all three of the NACKs, allowing us to continue discussion on the controversial part of Paul's patch while also allowing the non-controversial part to go in right away? Jan
On 10/9/19 11:16 AM, Jan Beulich wrote: > On 08.10.2019 18:38, Andrew Cooper wrote: >> On 08/10/2019 17:10, Jan Beulich wrote: >>> From: Paul Durrant <paul.durrant@citrix.com> >>> >>> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a >>> domain, migration may be needlessly vetoed due to the check of >>> is_iommu_enabled() in paging_log_dirty_enable(). >>> There is actually no need to prevent logdirty from being enabled unless >>> devices are assigned to a domain. >>> >>> NOTE: While in the neighbourhood, the bool_t parameter type in >>> paging_log_dirty_enable() is replaced with a bool and the format >>> of the comment in assign_device() is fixed. >>> >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> Release-acked-by: Juergen Gross <jgross@suse.com> >> >> Seriously FFS. Why am I having to repeat myself? What if any way >> unclear on the previous threads? >> >> NACK NACK NACK. > > With George having given his R-b I'm now in an awkward position: > I shouldn't ignore this triple NACK and commit the patch, but > there's also no sensible way forward here which would allow the > regression to be taken care of without committing the patch in > its current shape. Are you willing to take back all three of the > NACKs, allowing us to continue discussion on the controversial > part of Paul's patch while also allowing the non-controversial > part to go in right away? Regardless of the merits of the change Andy wants to see, it's not a one that should be made during a feature freeze. -George
On 09.10.19 12:21, George Dunlap wrote: > On 10/9/19 11:16 AM, Jan Beulich wrote: >> On 08.10.2019 18:38, Andrew Cooper wrote: >>> On 08/10/2019 17:10, Jan Beulich wrote: >>>> From: Paul Durrant <paul.durrant@citrix.com> >>>> >>>> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a >>>> domain, migration may be needlessly vetoed due to the check of >>>> is_iommu_enabled() in paging_log_dirty_enable(). >>>> There is actually no need to prevent logdirty from being enabled unless >>>> devices are assigned to a domain. >>>> >>>> NOTE: While in the neighbourhood, the bool_t parameter type in >>>> paging_log_dirty_enable() is replaced with a bool and the format >>>> of the comment in assign_device() is fixed. >>>> >>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> Release-acked-by: Juergen Gross <jgross@suse.com> >>> >>> Seriously FFS. Why am I having to repeat myself? What if any way >>> unclear on the previous threads? >>> >>> NACK NACK NACK. >> >> With George having given his R-b I'm now in an awkward position: >> I shouldn't ignore this triple NACK and commit the patch, but >> there's also no sensible way forward here which would allow the >> regression to be taken care of without committing the patch in >> its current shape. Are you willing to take back all three of the >> NACKs, allowing us to continue discussion on the controversial >> part of Paul's patch while also allowing the non-controversial >> part to go in right away? > > Regardless of the merits of the change Andy wants to see, it's not a one > that should be made during a feature freeze. Indeed. So either we take this patch or we have to revert the patch(es) introducing the regression. Juergen
On 10/9/19 11:23 AM, Jürgen Groß wrote: >> Regardless of the merits of the change Andy wants to see, it's not a one >> that should be made during a feature freeze. > > Indeed. So either we take this patch or we have to revert the patch(es) > introducing the regression. Actually, just chatting with Ian -- the worse issue ATM, AFAICT, is that the IOMMU is enabled for a guest which has neither asked for PCI devices, nor explicitly enabled it; and he's currently working on a fix for that. Once that issue is fixed, then osstest should become unblocked again. It is, arguably, not ideal to refuse to migrate a VM with IOMMU enabled but no devices attached; but if it only affected guests who had specifically requested the IOMMU be enabled, that wouldn't be so terrible. (And in fact it has highlighted the other, more important issue.) In summary, this patch is not strictly needed to get a push to osstest. That said, the behavior in 4.12 was, as far as I can tell: 1. If a guest had never had a PCI device assigned, Xen will allow logdirty to be enabled. 2. If a guest has a PCI device assigned, Xen will not allow logdirty to be enabled (blocking migration). 3. If a guest had a PCI device assigned in the past but does not have one now, Xen will also not allow logdirty to be enabled (blocking migration). At the moment, once Ian fixes the default, the behavior will be: 1 If a guest never had PCI device assigned, and a. the IOMMU was not explicitly enabled, Xen will allow logdirty to be enabled b. the IOMMU was explicitly enabled, xen will not allow logdirty 1a, 2, and 3 are the same, and 1b didn't exist in 4.12, so arguably there's no regression. This patch changes things so that migration will work in the 1b and 3 cases (if I'm reading it right). This is arguably better, but also arguably an unnecessary change post-freeze. And of course there's the option Andy is proposing, of having Xen allow logdirty to be enabled in all cases, and having the toolstack keep track of whether there are devices assigned and refuse migration if so; that's a technical change which should be avoided post-freeze. -George
On 09.10.2019 16:14, George Dunlap wrote: > On 10/9/19 11:23 AM, Jürgen Groß wrote: >>> Regardless of the merits of the change Andy wants to see, it's not a one >>> that should be made during a feature freeze. >> >> Indeed. So either we take this patch or we have to revert the patch(es) >> introducing the regression. > > Actually, just chatting with Ian -- the worse issue ATM, AFAICT, is that > the IOMMU is enabled for a guest which has neither asked for PCI > devices, nor explicitly enabled it; and he's currently working on a fix > for that. Once that issue is fixed, then osstest should become > unblocked again. > > It is, arguably, not ideal to refuse to migrate a VM with IOMMU enabled > but no devices attached; but if it only affected guests who had > specifically requested the IOMMU be enabled, that wouldn't be so > terrible. (And in fact it has highlighted the other, more important issue.) > > In summary, this patch is not strictly needed to get a push to osstest. > > That said, the behavior in 4.12 was, as far as I can tell: > > 1. If a guest had never had a PCI device assigned, Xen will allow > logdirty to be enabled. > > 2. If a guest has a PCI device assigned, Xen will not allow logdirty to > be enabled (blocking migration). > > 3. If a guest had a PCI device assigned in the past but does not have > one now, Xen will also not allow logdirty to be enabled (blocking > migration). No - the connection previously was to whether IOMMU page tables had been set up; these page tables would have been torn down upon de-assignment of the last device, allowing migration again. People actually use this behavior afaik, using a bond of a passed through SR-IOV NIC and netfront provided device. To migrate the VM, the SR-IOV NIC is taken away without the domain losing network access, and a new one might then be assigned again after migration. The "IOMMU page tables set up" property was previously identical to "has devices assigned", i.e. even before we could have used has_arch_pdevs() instead of is_iommu_enabled(). Jan
On 10/9/19 3:28 PM, Jan Beulich wrote: > On 09.10.2019 16:14, George Dunlap wrote: >> On 10/9/19 11:23 AM, Jürgen Groß wrote: >>>> Regardless of the merits of the change Andy wants to see, it's not a one >>>> that should be made during a feature freeze. >>> >>> Indeed. So either we take this patch or we have to revert the patch(es) >>> introducing the regression. >> >> Actually, just chatting with Ian -- the worse issue ATM, AFAICT, is that >> the IOMMU is enabled for a guest which has neither asked for PCI >> devices, nor explicitly enabled it; and he's currently working on a fix >> for that. Once that issue is fixed, then osstest should become >> unblocked again. >> >> It is, arguably, not ideal to refuse to migrate a VM with IOMMU enabled >> but no devices attached; but if it only affected guests who had >> specifically requested the IOMMU be enabled, that wouldn't be so >> terrible. (And in fact it has highlighted the other, more important issue.) >> >> In summary, this patch is not strictly needed to get a push to osstest. >> >> That said, the behavior in 4.12 was, as far as I can tell: >> >> 1. If a guest had never had a PCI device assigned, Xen will allow >> logdirty to be enabled. >> >> 2. If a guest has a PCI device assigned, Xen will not allow logdirty to >> be enabled (blocking migration). >> >> 3. If a guest had a PCI device assigned in the past but does not have >> one now, Xen will also not allow logdirty to be enabled (blocking >> migration). > > No - the connection previously was to whether IOMMU page tables > had been set up; these page tables would have been torn down > upon de-assignment of the last device, allowing migration again. > People actually use this behavior afaik, using a bond of a > passed through SR-IOV NIC and netfront provided device. To > migrate the VM, the SR-IOV NIC is taken away without the domain > losing network access, and a new one might then be assigned > again after migration. > > The "IOMMU page tables set up" property was previously identical > to "has devices assigned", i.e. even before we could have used > has_arch_pdevs() instead of is_iommu_enabled(). I didn't realize that. So yes, this patch fixes a regression over and above the toolstack fix Ian is working on. -George
--- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -71,7 +71,7 @@ int hap_track_dirty_vram(struct domain * if ( !paging_mode_log_dirty(d) ) { - rc = paging_log_dirty_enable(d, 0); + rc = paging_log_dirty_enable(d, false); if ( rc ) goto out; } --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -209,15 +209,15 @@ static int paging_free_log_dirty_bitmap( return rc; } -int paging_log_dirty_enable(struct domain *d, bool_t log_global) +int paging_log_dirty_enable(struct domain *d, bool log_global) { int ret; - if ( is_iommu_enabled(d) && log_global ) + if ( has_arch_pdevs(d) && log_global ) { /* * Refuse to turn on global log-dirty mode - * if the domain is using the IOMMU. + * if the domain is sharing the P2M with the IOMMU. */ return -EINVAL; } @@ -727,7 +727,7 @@ int paging_domctl(struct domain *d, stru break; /* Else fall through... */ case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY: - return paging_log_dirty_enable(d, 1); + return paging_log_dirty_enable(d, true); case XEN_DOMCTL_SHADOW_OP_OFF: if ( (rc = paging_log_dirty_disable(d, resuming)) != 0 ) --- a/xen/include/asm-x86/paging.h +++ b/xen/include/asm-x86/paging.h @@ -157,7 +157,7 @@ void paging_log_dirty_range(struct domai uint8_t *dirty_bitmap); /* enable log dirty */ -int paging_log_dirty_enable(struct domain *d, bool_t log_global); +int paging_log_dirty_enable(struct domain *d, bool log_global); /* log dirty initialization */ void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops);