On Tue, Jun 7, 2022 at 7:02 PM Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 6/7/22 16:35, Warner Losh wrote:
> >
> >
> >> On Jun 7, 2022, at 3:23 PM, Richard Henderson <
> richard.hender...@linaro.org> wrote:
> >>
> >> On 6/7/22 14:51, Warner Losh wrote:
> >>> void unlock_iovec(IOVecMap *map, bool copy_out)
> >>> {
> >>> for (int i = 0, count = map->count; i < count; ++i) {
> >>> if (map->host[i].iov_base) {
> >>> abi_ulong target_base =
> tswapal(map->target[i].iov_base);
> >>> unlock_user(map->host[i].iov_base, target_base,
> >>> copy_out ? map->host[i].iov_len : 0);
> >>> }
> >>> And wouldn't we want to filter out the iov_base that == 0 since
> >>> we may terminate the loop before we get to the count. When the
> >>> I/O is done, we'll call it not with the number we mapped, but with
> >>> the original number... Or am I not understanding something here...
> >>
> >> I'm not following -- when and why are you adjusting count?
> >
> > When we hit a memory range we can’t map after the first one,
> > we effectively stop mapping in (in the current linux code we
> > do map after, but then destroy the length). So that means
> > we’ll have entries in the iovec that are zero, and this code
> > doesn’t account for that. We’re not changing the count, per
> > se, but have a scenario where they might wind up NULL.
>
> ... and so skip them with the if.
>
> I mean, I suppose you could set map->count on error, as you say, so that
> we don't iterate
> so far, but... duh, error case. So long as you don't actively fail,
> there's no point in
> optimizing for it.
>
Setting the count would be hard because we'd have to allocate and free
state that we're not currently doing. Better to just skip it with an if. We
allocate
a vector that's used in a number of places, and we'd have to change that
code if we did things differently. While I'm open to suggestions here, I
think
that just accounting for the possible error with an if is our best bet for
now.
I have a lot of code to get in, and am hoping to not rewrite things unless
there's
some clear benefit over the existing structure (like fixing bugs, matching
linux-user,
or increasing performance).
Warner
