On 30.01.2023 05:35, Christopher Clark wrote: > On Mon, Nov 21, 2022 at 4:41 AM Jan Beulich <[email protected]> wrote: > >> On 11.10.2022 11:28, Jan Beulich wrote: >>> find_ring_mfn() already holds a page reference when trying to obtain a >>> writable type reference. We shouldn't make assumptions on the general >>> reference count limit being effectively "infinity". Obtain merely a type >>> ref, re-using the general ref by only dropping the previously acquired >>> one in the case of an error. >>> >>> Signed-off-by: Jan Beulich <[email protected]> >> >> Ping? > > Sorry it has taken me so long to review this patch and thank-you for > posting it. The points raised are helpful. > > Wrt to the patch - I can't ack because: > the general ref that is already held is from the page owner, and it may > actually be foreign; so the second ref acquire is currently ensuring that > it is a match for the owner of the ring. That needs addressing.
I'm afraid I may not understand your reply: Are you saying there's something wrong with the change? Or are you saying there's something wrong that merely becomes apparent due to the change? Or yet something else? > Am supportive of points raised: > - review + limit ref counts taken > - better to not need two general page refs > - a type ref rather than general may be sufficient to hold for the ring > lifetime? > - paging_mark_dirty at writes > - p2m log dirty would be better to be allowed than EAGAIN I can certainly extend the patch to a series, but that'll make sense only if ... > - allowing mapping of foreign pages may have uses though likely also > challenging > > I should let you know that my time available is extremely limited at the > moment, sorry. ... it would then also be looked at within a reasonable amount of time. I'm already sitting on way too many unreviewed patches. Jan
