Jonathan Davies
2011-Nov-14 11:06 UTC
[Xen-devel] [PATCH] [OCaml] Release the global lock during some hypercalls
Since libxc is re-entrant, there is no need for the OCaml bindings to prevent
more than one thread from entering libxc concurrently.
Previously, the OCaml bindings had prevented re-entrancy by not using
caml_{enter,leave}_blocking_section in the C stubs. The absence of these calls
meant that the global lock remained held during hypercalls. This caused
multi-threaded applications to completely lock up during long-running
hypercalls.
Calls to these functions were present but commented out in the OCaml bindings
some years ago when libxc was not fully re-entrant.
Instead, we now do call caml_{enter,leave}_blocking_section in all the places it
used to be commented out, meaning that the global lock is released during those
hypercalls. We also no longer assert the XC_OPENFLAG_NON_REENTRANT flag when
calling xc_interface_open because the caller no longer does re-entrancy
prevention at those places.
Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
diff -r 0844b17df7a9 -r 10758a9aa01c tools/ocaml/libs/xc/xenctrl_stubs.c
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c Fri Nov 11 18:14:35 2011 +0000
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c Mon Nov 14 11:03:04 2011 +0000
@@ -115,7 +115,10 @@ CAMLprim value stub_xc_interface_open(vo
{
CAMLparam0();
xc_interface *xch;
- xch = xc_interface_open(NULL, NULL, XC_OPENFLAG_NON_REENTRANT);
+
+ /* Don''t assert XC_OPENFLAG_NON_REENTRANT because these bindings
+ * do not prevent re-entrancy to libxc */
+ xch = xc_interface_open(NULL, NULL, 0);
if (xch == NULL)
failwith_xc(NULL);
CAMLreturn((value)xch);
@@ -133,9 +136,9 @@ CAMLprim value stub_xc_interface_close(v
{
CAMLparam1(xch);
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
xc_interface_close(_H(xch));
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
CAMLreturn(Val_unit);
}
@@ -170,9 +173,9 @@ CAMLprim value stub_xc_domain_create(val
c_flags |= domain_create_flag_table[v];
}
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
result = xc_domain_create(_H(xch), c_ssidref, h, c_flags, &domid);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (result < 0)
failwith_xc(_H(xch));
@@ -217,12 +220,13 @@ value stub_xc_domain_sethandle(value xch
static value dom_op(value xch, value domid, int (*fn)(xc_interface *,
uint32_t))
{
CAMLparam2(xch, domid);
+ int result;
uint32_t c_domid = _D(domid);
- // caml_enter_blocking_section();
- int result = fn(_H(xch), c_domid);
- // caml_leave_blocking_section();
+ caml_enter_blocking_section();
+ result = fn(_H(xch), c_domid);
+ caml_leave_blocking_section();
if (result)
failwith_xc(_H(xch));
CAMLreturn(Val_unit);
@@ -247,12 +251,13 @@ CAMLprim value stub_xc_domain_destroy(va
CAMLprim value stub_xc_domain_resume_fast(value xch, value domid)
{
CAMLparam2(xch, domid);
+ int result;
uint32_t c_domid = _D(domid);
- // caml_enter_blocking_section();
- int result = xc_domain_resume(_H(xch), c_domid, 1);
- // caml_leave_blocking_section();
+ caml_enter_blocking_section();
+ result = xc_domain_resume(_H(xch), c_domid, 1);
+ caml_leave_blocking_section();
if (result)
failwith_xc(_H(xch));
CAMLreturn(Val_unit);
@@ -324,10 +329,10 @@ CAMLprim value stub_xc_domain_getinfolis
c_first_domain = _D(first_domain);
c_max_domains = Int_val(nb);
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
retval = xc_domain_getinfolist(_H(xch), c_first_domain,
c_max_domains, info);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval < 0) {
free(info);
@@ -372,10 +377,10 @@ CAMLprim value stub_xc_vcpu_getinfo(valu
uint32_t c_domid = _D(domid);
uint32_t c_vcpu = Int_val(vcpu);
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
retval = xc_vcpu_getinfo(_H(xch), c_domid,
c_vcpu, &info);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval < 0)
failwith_xc(_H(xch));
@@ -492,14 +497,15 @@ CAMLprim value stub_xc_evtchn_alloc_unbo
value remote_domid)
{
CAMLparam3(xch, local_domid, remote_domid);
+ int result;
uint32_t c_local_domid = _D(local_domid);
uint32_t c_remote_domid = _D(remote_domid);
- // caml_enter_blocking_section();
- int result = xc_evtchn_alloc_unbound(_H(xch), c_local_domid,
+ caml_enter_blocking_section();
+ result = xc_evtchn_alloc_unbound(_H(xch), c_local_domid,
c_remote_domid);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (result < 0)
failwith_xc(_H(xch));
@@ -525,12 +531,13 @@ CAMLprim value stub_xc_readconsolering(v
{
unsigned int size = RING_SIZE - 1;
char *ring_ptr = ring;
+ int retval;
CAMLparam1(xch);
- // caml_enter_blocking_section();
- int retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
- // caml_leave_blocking_section();
+ caml_enter_blocking_section();
+ retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
+ caml_leave_blocking_section();
if (retval)
failwith_xc(_H(xch));
@@ -557,9 +564,9 @@ CAMLprim value stub_xc_physinfo(value xc
xc_physinfo_t c_physinfo;
int r;
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
r = xc_physinfo(_H(xch), &c_physinfo);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (r)
failwith_xc(_H(xch));
@@ -603,9 +610,9 @@ CAMLprim value stub_xc_pcpu_info(value x
if (!info)
caml_raise_out_of_memory();
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
r = xc_getcpuinfo(_H(xch), Int_val(nr_cpus), info, &size);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (r) {
free(info);
@@ -629,13 +636,14 @@ CAMLprim value stub_xc_domain_setmaxmem(
value max_memkb)
{
CAMLparam3(xch, domid, max_memkb);
+ int retval;
uint32_t c_domid = _D(domid);
unsigned int c_max_memkb = Int64_val(max_memkb);
- // caml_enter_blocking_section();
- int retval = xc_domain_setmaxmem(_H(xch), c_domid,
+ caml_enter_blocking_section();
+ retval = xc_domain_setmaxmem(_H(xch), c_domid,
c_max_memkb);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval)
failwith_xc(_H(xch));
CAMLreturn(Val_unit);
@@ -661,14 +669,15 @@ CAMLprim value stub_xc_domain_memory_inc
value mem_kb)
{
CAMLparam3(xch, domid, mem_kb);
+ int retval;
unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >>
(PAGE_SHIFT - 10);
uint32_t c_domid = _D(domid);
- // caml_enter_blocking_section();
- int retval = xc_domain_increase_reservation_exact(_H(xch), c_domid,
+ caml_enter_blocking_section();
+ retval = xc_domain_increase_reservation_exact(_H(xch), c_domid,
nr_extents, 0, 0, NULL);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval)
failwith_xc(_H(xch));
@@ -796,10 +805,10 @@ CAMLprim value stub_xc_version_version(v
long packed;
int retval;
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
packed = xc_version(_H(xch), XENVER_version, NULL);
retval = xc_version(_H(xch), XENVER_extraversion, &extra);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval)
failwith_xc(_H(xch));
@@ -821,9 +830,9 @@ CAMLprim value stub_xc_version_compile_i
xen_compile_info_t ci;
int retval;
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
retval = xc_version(_H(xch), XENVER_compile_info, &ci);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval)
failwith_xc(_H(xch));
@@ -844,9 +853,9 @@ static value xc_version_single_string(va
CAMLparam1(xch);
int retval;
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
retval = xc_version(_H(xch), code, info);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (retval)
failwith_xc(_H(xch));
@@ -895,11 +904,11 @@ CAMLprim value stub_map_foreign_range(va
c_dom = _D(dom);
c_mfn = Nativeint_val(mfn);
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
intf->addr = xc_map_foreign_range(_H(xch), c_dom,
intf->len, PROT_READ|PROT_WRITE,
c_mfn);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (!intf->addr)
caml_failwith("xc_map_foreign_range error");
CAMLreturn(result);
@@ -912,9 +921,9 @@ CAMLprim value stub_sched_credit_domain_
struct xen_domctl_sched_credit c_sdom;
int ret;
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
ret = xc_sched_credit_domain_get(_H(xch), _D(domid), &c_sdom);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (ret != 0)
failwith_xc(_H(xch));
@@ -934,9 +943,9 @@ CAMLprim value stub_sched_credit_domain_
c_sdom.weight = Int_val(Field(sdom, 0));
c_sdom.cap = Int_val(Field(sdom, 1));
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
ret = xc_sched_credit_domain_set(_H(xch), _D(domid), &c_sdom);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (ret != 0)
failwith_xc(_H(xch));
@@ -950,11 +959,11 @@ CAMLprim value stub_shadow_allocation_ge
unsigned long c_mb;
int ret;
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
ret = xc_shadow_control(_H(xch), _D(domid),
XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION,
NULL, 0, &c_mb, 0, NULL);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (ret != 0)
failwith_xc(_H(xch));
@@ -970,11 +979,11 @@ CAMLprim value stub_shadow_allocation_se
int ret;
c_mb = Int_val(mb);
- // caml_enter_blocking_section();
+ caml_enter_blocking_section();
ret = xc_shadow_control(_H(xch), _D(domid),
XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
NULL, 0, &c_mb, 0, NULL);
- // caml_leave_blocking_section();
+ caml_leave_blocking_section();
if (ret != 0)
failwith_xc(_H(xch));
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Nov-22 18:48 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
Jonathan Davies writes ("[Xen-devel] [PATCH] [OCaml] Release the global
lock during some hypercalls"):> Since libxc is re-entrant, there is no need for the OCaml bindings to
prevent more than one thread from entering libxc concurrently.
Well, libxc is supposed to be re-entrant apart from certain calls,
yes.
As the main user of the ocaml bindings is xapi, can you confirm that
you''ve run this change through a good test of xapi (eg, the Citrix
XenRT suite) ?
Thanks,
Ian.
Ian Campbell
2011-Nov-23 09:46 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
On Tue, 2011-11-22 at 18:48 +0000, Ian Jackson wrote:> Jonathan Davies writes ("[Xen-devel] [PATCH] [OCaml] Release the global lock during some hypercalls"): > > Since libxc is re-entrant, there is no need for the OCaml bindings to prevent more than one thread from entering libxc concurrently. > > Well, libxc is supposed to be re-entrant apart from certain calls, > yes.Which ones aren''t? Either I''m grepping the wrong thing in xenctrl.h or it is documented somewhere else (OK, I admit it, there''s a third, more likely, option...)> > As the main user of the ocaml bindings is xapi, can you confirm that > you''ve run this change through a good test of xapi (eg, the Citrix > XenRT suite) ? > > Thanks, > Ian. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel
Jonathan Davies
2011-Nov-23 09:47 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
Ian Jackson wrote:> Jonathan Davies writes ("[Xen-devel] [PATCH] [OCaml] Release the global > lock during some hypercalls"): > > Since libxc is re-entrant, there is no need for the OCaml bindings to > prevent more than one thread from entering libxc concurrently. > > Well, libxc is supposed to be re-entrant apart from certain calls, > yes. > > As the main user of the ocaml bindings is xapi, can you confirm that > you''ve run this change through a good test of xapi (eg, the Citrix > XenRT suite) ?It hasn''t yet gone through XenRT, although I''ve done some manual xapi stress testing in addition to some basic functional verification tests. It sounds like you''d prefer it to go through XenRT before accepting it here. I''ll do so, and then confirm if all is well. Thanks, Jonathan
Ian Jackson
2011-Nov-24 18:43 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
Ian Campbell writes ("Re: [Xen-devel] [PATCH] [OCaml] Release the global
lock du> Which ones aren''t? Either I''m grepping the wrong
thing in xenctrl.h or> it is documented somewhere else (OK, I admit it, there''s a third,
more
> likely, option...)
As an example, the migration calls aren''t reentrant. Not because of
clashes in the libxc data structures but because they do things to the
domain. There are probably a few others but they''re probably not
worth worrying about ...
> > As the main user of the ocaml bindings is xapi, can you confirm that
> > you''ve run this change through a good test of xapi (eg, the
Citrix
> > XenRT suite) ?
... if the result passes a reasonable test suite.
Ian.
Ian Jackson
2011-Nov-24 18:46 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
Jonathan Davies writes ("Re: [Xen-devel] [PATCH] [OCaml] Release the global
lock during some hypercalls"):> It hasn''t yet gone through XenRT, although I''ve done some
manual xapi stress testing in addition to some basic functional verification
tests.
>
> It sounds like you''d prefer it to go through XenRT before
accepting it here. I''ll do so, and then confirm if all is well.
I think that would be useful, if that''s OK with you.
Ian.
Jonathan Davies
2011-Nov-29 10:14 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
Ian Jackson wrote:> Jonathan Davies writes ("Re: [Xen-devel] [PATCH] [OCaml] Release the > global lock during some hypercalls"): > > It hasn''t yet gone through XenRT, although I''ve done some manual xapi > > stress testing in addition to some basic functional verification > > tests. > > > > It sounds like you''d prefer it to go through XenRT before accepting > > it here. I''ll do so, and then confirm if all is well. > > I think that would be useful, if that''s OK with you.The patch has now gone through a XenRT nightly test; no problems were observed. Is that sufficient to allay your potential concern? Thanks, Jonathan
Ian Jackson
2011-Dec-01 17:26 UTC
Re: [PATCH] [OCaml] Release the global lock during some hypercalls
Jonathan Davies writes ("Re: [Xen-devel] [PATCH] [OCaml] Release the global
lock during some hypercalls"):> The patch has now gone through a XenRT nightly test; no problems were
observed. Is that sufficient to allay your potential concern?
Yes, thanks, I have commited the patch.
Ian.