The first four patches can be attributed to the former, the last four patches to the latter. The middle patch had been submitted standalone before, has a suitable Reviewed-by tag, but also has an objection by Andrew pending, which unfortunately has lead to this patch now being stuck. Short of Andrew being willing to settle the disagreement more with Julien than with me (although I'm on Julien's side), I have no idea what to do here. There's probably not much interrelation between the patches, so they can perhaps go in about any order. 1: defer allocation of maptrack frames table 2: drop a redundant expression from gnttab_release_mappings() 3: fold recurring is_iomem_page() 4: drop GNTMAP_can_fail 5: defer allocation of status frame tracking array 6: check handle early in gnttab_get_status_frames() 7: no need to translate handle for gnttab_get_status_frames() 8: bail from GFN-storing loops early in case of error 9: don't silently truncate GFNs in compat setup-table handling Jan
On 26.08.2021 12:06, Jan Beulich wrote: > The first four patches can be attributed to the former, the last four > patches to the latter. The middle patch had been submitted standalone > before, has a suitable Reviewed-by tag, but also has an objection by > Andrew pending, which unfortunately has lead to this patch now being > stuck. Short of Andrew being willing to settle the disagreement more > with Julien than with me (although I'm on Julien's side), I have no > idea what to do here. > > There's probably not much interrelation between the patches, so they > can perhaps go in about any order. > > 1: defer allocation of maptrack frames table > 2: drop a redundant expression from gnttab_release_mappings() > 3: fold recurring is_iomem_page() > 4: drop GNTMAP_can_fail > 5: defer allocation of status frame tracking array Just to make "official" what I've said in the course of the resource management discussion at the event in Cambridge: I'm withdrawing 1 and 5, in the expectation that eager/lazy allocation of resources will become a property to be honored uniformly for a guest. With 2, 3, 4, and 6 already having gone in, it would still be nice to (finally) have feedback on ... > 6: check handle early in gnttab_get_status_frames() > 7: no need to translate handle for gnttab_get_status_frames() > 8: bail from GFN-storing loops early in case of error > 9: don't silently truncate GFNs in compat setup-table handling ... the remaining three - even if these aren't really 4.17 candidates anymore at this point. Thanks, Jan
On 07/10/2022 14:49, Jan Beulich wrote: > On 26.08.2021 12:06, Jan Beulich wrote: >> The first four patches can be attributed to the former, the last four >> patches to the latter. The middle patch had been submitted standalone >> before, has a suitable Reviewed-by tag, but also has an objection by >> Andrew pending, which unfortunately has lead to this patch now being >> stuck. Short of Andrew being willing to settle the disagreement more >> with Julien than with me (although I'm on Julien's side), I have no >> idea what to do here. >> >> There's probably not much interrelation between the patches, so they >> can perhaps go in about any order. >> >> 1: defer allocation of maptrack frames table >> 2: drop a redundant expression from gnttab_release_mappings() >> 3: fold recurring is_iomem_page() >> 4: drop GNTMAP_can_fail >> 5: defer allocation of status frame tracking array > Just to make "official" what I've said in the course of the resource > management discussion at the event in Cambridge: I'm withdrawing 1 > and 5, in the expectation that eager/lazy allocation of resources > will become a property to be honored uniformly for a guest. With 2, > 3, 4, and 6 already having gone in, it would still be nice to > (finally) have feedback on ... To these issues in particular, there was specific work done in 4.16 to address the impasse in a way which got the savings in most cases, but without increasing the risk of hitting known buggy paths in guest drivers. For patch 5, the ABI max version is now known to domain_create(), so the status array allocation can safely be skipped when gnttab v2 isn't available. This gets the saving Julien wants on ARM, and on x86 we ought to encourage people to restricting to v1 where possible, so they get the savings too. For patch 1, I agree with the observation that 1024 maptrack frames is a silly default to have. Adjusting this appropriately will result in the kind of savings wanted in the patch, without modifying the hypervisor directly. We should have a patch to xl (or is it libxl?) to make a better choice than blindly picking 1024. Having written this out, something does strike me as odd. These limits are specified in frames, and for the gnttab limit, this equates into a known number of grants. But struct maptrack is internal Xen, so the toolstack selecting 1024 frames can't know how many grant maps that actually equates to in Xen. Right now, 1024 maptrack frames equates to 2^18 mappings (I think). For traditional server-virt VMs, they can have 0 because the frontends should never be mapping grants. We actually already do this for the xenstored stubdom. The same is almost certainly true for qemu stubdoms. For VMs in a bit of a more interesting configuration, e.g. with virtio, then they need "some". For device driver VMs, they do need a dom0-like mapping capabilities. Either way, there is definitely room to improve the status quo without impacting runtime safety. If patches were to appear promptly, I think there's probably wiggle room to consider them for 4.17 at this point. ~Andrew
On 26.08.2021 12:06, Jan Beulich wrote: > The first four patches can be attributed to the former, the last four > patches to the latter. The middle patch had been submitted standalone > before, has a suitable Reviewed-by tag, but also has an objection by > Andrew pending, which unfortunately has lead to this patch now being > stuck. Short of Andrew being willing to settle the disagreement more > with Julien than with me (although I'm on Julien's side), I have no > idea what to do here. > > There's probably not much interrelation between the patches, so they > can perhaps go in about any order. > > 1: defer allocation of maptrack frames table > 2: drop a redundant expression from gnttab_release_mappings() > 3: fold recurring is_iomem_page() > 4: drop GNTMAP_can_fail > 5: defer allocation of status frame tracking array > 6: check handle early in gnttab_get_status_frames() > 7: no need to translate handle for gnttab_get_status_frames() > 8: bail from GFN-storing loops early in case of error > 9: don't silently truncate GFNs in compat setup-table handling While some of the patches have gone in, and while I realize patches 1 and 5 are controversial (with no clear route towards resolution), may I ask for acks or otherwise on patches 7, 8, and 9 please? Thanks, Jan
© 2016 - 2024 Red Hat, Inc.