On Thu, Jun 09, 2022 at 11:03:02AM -0500, Eric Blake wrote:
> On Thu, Jun 09, 2022 at 03:22:42PM +0100, Richard W.M. Jones wrote:
> > On Thu, Jun 09, 2022 at 08:34:43AM -0500, Eric Blake wrote:
> > > Instead of open-coding our code to create a PyObject wrapper of the
> > > mutable *error to be passed to each callback, extract this into a
> > > helper function. We can then slightly optimize things to hang on to a
> > > single pointer to the ctypes module, rather than looking it up on
> > > every callback. The generated code shrinks by more than the added
> > > code in utils.c:
> > >
>
> > > +++ b/generator/Python.ml
> > > @@ -41,6 +41,7 @@ let
> > > extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
> > > extern int nbd_internal_py_init_aio_buffer (PyObject *);
> > > extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
> > > +extern PyObject *nbd_internal_py_wrap_errptr (int);
> > >
> > > static inline struct nbd_handle *
> > > get_handle (PyObject *obj)
> > > @@ -184,13 +185,7 @@ let
> > > | CBInt _
> > > | CBInt64 _ -> ()
> > > | CBMutable (Int n) ->
> > > - pr " PyObject *py_%s_modname = PyUnicode_FromString
> > > (\"ctypes\");\n" n;
> > > - pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n" n;
> > > - pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n
> > > n;
> > > - pr " Py_DECREF (py_%s_modname);\n" n;
> > > - pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n;
> > > - pr " py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\",
> > > *%s);\n" n n n;
> > > - pr " Py_DECREF (py_%s_mod);\n" n;
>
> The old code was using PyObject_CallMethod outside the GIL lock...
>
> > > + pr " py_%s = nbd_internal_py_wrap_errptr (*%s);\n" n n;
> > > pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n;
> > > | CBString _
> > > | CBUInt _
> > > diff --git a/python/utils.c b/python/utils.c
> > > index e0df181..cd44b90 100644
> > > --- a/python/utils.c
> > > +++ b/python/utils.c
> > > @@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
> > > }
> > > return type;
> > > }
> > > +
> > > +/* Helper to package callback *error into modifiable PyObject */
> > > +PyObject *
> > > +nbd_internal_py_wrap_errptr (int err)
> > > +{
> > > + static PyObject *py_ctypes_mod;
> > > +
> > > + if (!py_ctypes_mod) {
> > > + PyObject *py_modname = PyUnicode_FromString ("ctypes");
> > > + if (!py_modname)
> > > + return NULL;
> > > + py_ctypes_mod = PyImport_Import (py_modname);
> > > + Py_DECREF (py_modname);
> > > + if (!py_ctypes_mod)
> > > + return NULL;
>
> ...the new code as well.
>
> > > + }
> > > +
> > > + return PyObject_CallMethod (py_ctypes_mod, "c_int", "i", err);
> > > +}
> >
> > I guess because of the Python GIL we don't need locking here?
>
> But you're right that any time we call into the python interpreter for
> something that may entail quite some level of complexity, the GIL lock
> needs to be worried about. I'll have to audit this and other recent
> patches to see if we need to add more use of
> PyGILState_{Ensure/Release} around various code blocks (after first
> researching what the lock is supposed to do...).
Okay, after reading more about the GIL, it looks like we are (likely)
safe only because we have never really been multi-threaded: none of
our libnbd code uses Py_BEGIN_ALLOW_THREADS to release the GIL around
our calls into libnbd C code, and therefore even though our placement
of PyGILState_Ensure() was too late, all of our callbacks have been
reached from places where we already had the GIL (well, hopefully
that's true). At any rate, I'm working on a patch to explicitly
release the GIL around calls into libnbd C code, which means we DO
need to fix our callback wrappers to grab it at the right point. But
the code in utils.c is indeed safe because of the GIL.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
