David Hildenbrand wrote:
> So, instead of pinning explicitly, look up the page address, cache it,
> and glue its lifetime to the gmap table entry. When that entry is
> changed, invalidate the cached page. On re-access, look up the page
> again and register the gmap notifier for the table entry again.
Yes, exactly.
> [...]> +static struct page *get_map_page(struct kvm *kvm,
> > + struct s390_io_adapter *adapter,
> > + u64 addr)
> > {
> > struct s390_map_info *map;
> > + unsigned long uaddr;
> > + struct page *page;
> > + bool need_retry;
> > + int ret;
> >
> > if (!adapter)
> > return NULL;
> > +retry:
> > + page = NULL;
> > + uaddr = 0;
> > + spin_lock(&adapter->maps_lock);
> > + list_for_each_entry(map, &adapter->maps, list)
> > + if (map->guest_addr == addr) {
>
> Could it happen, that we don't have a fitting entry in the list?
Yes, if user space tries to signal an interrupt on a page that
was not properly announced via KVM_S390_IO_ADAPTER_MAP.
In that case, the loop returns with page == NULL and uaddr == 0,
which will cause the code below to return NULL, which will cause
the caller to return an error to user space.
> > + uaddr = map->addr;
> > + page = map->page;
> > + if (!page)
> > + map->page = ERR_PTR(-EBUSY);
> > + else if (IS_ERR(page) || !page_cache_get_speculative(page)) {
> > + spin_unlock(&adapter->maps_lock);
> > + goto retry;
> > + }
> > + break;
> > + }
>
> Can we please factor out looking up the list entry to a separate
> function, to be called under lock? (and e.g., use it below as well)
Good idea, I like that. Will update the patch ...
> > + need_retry = true;
> > + spin_lock(&adapter->maps_lock);
> > + list_for_each_entry(map, &adapter->maps, list)
> > + if (map->guest_addr == addr) {
>
> Could it happen that our entry is suddenly no longer in the list?
Yes, if user space did a KVM_S390_IO_ADAPTER_UNMAP in the meantime.
In this case we'll exit the loop with need_retry == true and will
restart from the beginning, usually then returning an error back
to user space.
> > + if (map->page == ERR_PTR(-EBUSY)) {
> > + map->page = page;
> > + need_retry = false;
> > + } else if (IS_ERR(map->page)) {
>
> else if (map->page == ERR_PTR(-EINVAL)
>
> or simpy "else" (every other value would be a BUG_ON, right?)
Usually yes. I guess there's the theoretical case that we race
with user space removing the old entry with KVM_S390_IO_ADAPTER_UNMAP
and immediately afterwards installing a new entry with the same
guest address. In that case, we'll also fall into the need_retry
case here.
> Wow, this function is ... special. Took me way to long to figure out
> what is going on here. We certainly need comments in there.
I agree. As Christian said, it's not fully clear that all of this
is really needed. Maybe just doing the get_user_pages_remote every
time is actually enough -- we should do the "cache" magic only if
this is really critical for performance.
> I can see that
>
> - ERR_PTR(-EBUSY) is used when somebody is about to do the
> get_user_pages_remote(). others have to loop until that is resolved.
> - ERR_PTR(-EINVAL) is used when the entry gets invalidated by the
> notifier while somebody is about to set it (while still
> ERR_PTR(-EBUSY)). The one currently processing the entry will
> eventually set it back to NULL.
Yes, that's the intent.
> I think we should make this clearer by only setting ERR_PTR(-EINVAL) in
> the notifier if already ERR_PTR(-EBUSY), along with a comment.
I guess I wanted to catch the case where we get another invalidation
while we already have -EINVAL. But given the rest of the logic, this
shouldn't actually ever happen. (If it *did* happen, however, then
setting to -EINVAL again is safer than resetting to NULL.)
> Can we document the values for map->page and how they are to be handled
> right in the struct?
OK, will do.
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
Ulrich.Weigand@xxxxxxxxxx
