Re: [PATCH 02/35] KVM: s390/interrupt: do not pin adapter interrupt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux