tools/libxl - Async Task Cancellation Query

tools/libxl - Async Task Cancellation Query

[Koushik Chakravarty]

[-- Attachment #1.1: Type: text/plain, Size: 1061 bytes --]

Hi Ian,

I am currently looking into the asynchronous task cancellation in libxl and have a few very specific queries, if you could answer.


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

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.

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.

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


Thanks in advance!

Regards,
Koushik Chakravarty
Mobile - +91-9663396424


[-- Attachment #1.2: Type: text/html, Size: 8045 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: tools/libxl - Async Task Cancellation Query

[Ian Jackson]

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.

Re: tools/libxl - Async Task Cancellation Query

[Koushik Chakravarty]

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@eu.citrix.com] 
Sent: Wednesday, April 8, 2015 4:52 PM
To: Koushik Chakravarty
Cc: Euan Harris; 'xen-devel@lists.xensource.com'
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.

Re: tools/libxl - Async Task Cancellation Query

[Koushik Chakravarty]

Hi Ian,

Any thoughts on this below?

Regards,
Koushik Chakravarty
Mobile - +91-9663396424

-----Original Message-----
From: Koushik Chakravarty 
Sent: Wednesday, April 8, 2015 5:44 PM
To: Ian Jackson
Cc: Euan Harris; 'xen-devel@lists.xensource.com'
Subject: RE: tools/libxl - Async Task Cancellation Query

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. Of course, the assumption here is that the caller keeps the ao_how struct and uses that to call libxl_ao_cancel() - but I see that is already the case.

Let me know if I am not being very clear.

Regards,
Koushik Chakravarty
Mobile - +91-9663396424

-----Original Message-----
From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com] 
Sent: Wednesday, April 8, 2015 4:52 PM
To: Koushik Chakravarty
Cc: Euan Harris; 'xen-devel@lists.xensource.com'
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.

Re: tools/libxl - Async Task Cancellation Query [and 1 more messages]

[Ian Jackson]

Koushik Chakravarty writes ("RE: tools/libxl - Async Task Cancellation Query"):
> 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? 

By the caller I mean the program outside libxl which is calling libxl.

> >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.

Yes.

> 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. 

Sorry, I had misunderstood.  I thought you were proposing some kind of
arrangement where libxl would be able to look up the relevant ao in
O(1).  That would require either the lifetime of these ids to be
controlled.

But now I understand that you're just proposing a pure id which still
requires a search.  I don't think the performance benefit is really
worthwhile for the extra complexity.

This is particularly true given that currently the ao_how is const.
So the caller might have defined it as `static const' and put it in
the text segment.  Changing the API to make this parameter non-const
sometimes, in a backward compatible way, would be a bit complicated.

But thanks anyway for your suggestion.

Ian.