Re: [Xen-devel] tools/libxl - Async Task Cancellation Query

[Koushik Chakravarty <koushik.chakravarty@xxxxxxxxxx>]

Thanks Ian for the answers. I have a follow - up on the below:

"Can I suggest adding a unique private 'id' field to the libxl_asyncop_how 
structure, that will be populated by AO_CREATE? This will help finding the 
matching corresponding libxl_ao from the ctx->aos_inprogress in 
libxl_ao_cancel() quicker by looking for search->id == libxl_asyncop_how->id.

That would require the caller to preserve the ao_how which seems awkward to me. 
 Also, allocating these ids presents some difficulties.
I think it is better to allow the caller to identify aos."


When you say - " That would require the caller to preserve the ao_how which 
seems awkward to me " - whom do you refer as the caller? 
From what I picked up, the libxl_ao_cancel() API is passed with the ao_how. The 
libxl_ao_cancel then looks into the ctx->aos_inprogress list to find the ao 
that matches the ao_how.
So what I was suggesting was that if the ao_how was allotted an 'id' from the 
original libxl api call (done using a counter increment from the global libxl 
ctx with a lock held), and the same id was saved in the ao entry in 
ctx->aos_inprogress, then searching/matching them in libxl_ao_cancel() would 
have been a little faster. 

Let me know if I am not being very clear.

Regards,
Koushik Chakravarty
Mobile - +91-9663396424

-----Original Message-----
From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx] 
Sent: Wednesday, April 8, 2015 4:52 PM
To: Koushik Chakravarty
Cc: Euan Harris; 'xen-devel@xxxxxxxxxxxxxxxxxxx'
Subject: Re: tools/libxl - Async Task Cancellation Query

Koushik Chakravarty writes ("tools/libxl - Async Task Cancellation Query"):
> I am currently looking into the asynchronous task cancellation in libxl and 
> have a few very specific queries, if you could answer.

Sure.  Thanks for what has evidently been a careful review of the code.

> 1.    In libxl_domain_resume(),why is libxl_ao_complete called before 
> AO_INPROGRESS?

I was going to refer you to the internal API documentation, but I find that it 
doesn't describe this situation.  I have drafted a patch to the documentation 
which I hope will answer this question.  I'll send it as a followup to this 
mail.

> 2.    In libxl_ao_cancel() - the function goes through the 
> ctx->aos_inprogress and tries to find a suitable libxl_ao that matches the 
> input libxl_asyncop_how. It does so, by a few 'if' checks. Regarding this -
> 
> a.    Where does the libxl__ao get inserted to the ctx->aos_inprogress? I 
> could not find that somehow - sorry if I overlooked.

Thank you again for your review.  I think you have spotted a bug.

I will send a followup patch, again as a followup to this mail.


> b.    Can I suggest adding a unique private 'id' field to the 
> libxl_asyncop_how structure, that will be populated by AO_CREATE? This will 
> help finding the matching corresponding libxl_ao from the ctx->aos_inprogress 
> in libxl_ao_cancel() quicker by looking for search->id == 
> libxl_asyncop_how->id.

That would require the caller to preserve the ao_how which seems awkward to me. 
 Also, allocating these ids presents some difficulties.
I think it is better to allow the caller to identify aos.


> 3.    In libxl_device_vkb_add(), shouldn't the function invoke 
> libxl__ao_abort in the error path?

I think this will be answered by my documentation patch.  But, to summarise, 
the codepaths:

    assert(rc);
    libxl__ao_complete(egc, ao, rc);
    return AO_INPROGRESS;

and

    assert(rc);
    return AO_ABORT(rc);

are equivalent.

Ian.

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