Re: [Xen-devel] Grant access to more than one dom

[Simon Martin <furryfuttock@xxxxxxxxx>]

Hello Daniel,

>>
>> 3.- Analyzing the return value of __release_gref I found there is a
>> possible (unlikely) infinite loop in the original __del_gref
>> implementation. If gref_id <= 0 then the list element will never be
>> released. If we have this situation then a memory leak is the the
>> least of our worries as there is either a problem in the logic or
>> memory corruption. I propose that under this situation we should just
>> return OK and carry on. What do you think?

> The infinite loop you added is incorrect and is actually quite likely to
> be triggered if the other domain is not responding to whatever unmap
> request has been set up.  This does not have to be an error; it could be
> triggered because the other domain has not yet been scheduled after the
> notify was queued.  If __release_gref fails, then you need to return from
> __del_gref without actually deleting the gref object.  This postpones the
> actual deletion until it is retried by do_cleanup.

> Encountering gref <= 0 in this loop should not happen; that indicates a
> significantproblem and deserves a WARN_ON if you want to check for it.

I have added WARN_ON(gref <= 0)

Removed loop waiting for foreign dom to release page and added
do_cleanup to end of share/unshare IOCTL.

>>> It would be nice if this function also supported freeing the initial grant
>>> reference, so that it can be used to change the domain ID a page is being
>>> shared with.
>>
>> The idea of gntalloc_gref.shares is that there is 1 or more grefs
>> associated to any allocation. This would mean changing that to 0 or
>> more, i.e. gntalloc_gref.shares should be changed directly to a list
>> head and we will require a memory allocation for the original
>> allocation. The change is simple, shall I do it?

> I would instead change the logic to something like:
> If removing the primary reference and there are secondary references, promote
> the first secondary reference (and remove/free its gntalloc_share_list). If
> removing the last reference, either error, act as DEALLOC_GREF, or change all
> other uses of gref_id to handle gref_id==0 meaning "not shared".

Implemented this. Not as clean as changing the semantics but avoids
dynamic allocation for the most common case.

> The name change to xen/gntalloc_trio should not be in the submitted patch;
> I assume this was for testing.

Sorry. I do this so I can have the original driver loaded at the same
time. I realized this after I'd sent the patch, but I'd already sent
it.

-- 
Best regards,
 Simon                            mailto:furryfuttock@xxxxxxxxx

Attachment: gntalloc.patch
Description: Binary data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel