Re: [libvirt] [PATCH 16/17] nwfilter: Remove recursive locking for nwfilter instantiation
On 07/13/2017 10:40 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
> The current algorithm required usage of recursive locks because there
> was no other mechanism available to ensure that the object wasn't deleted
> whilst the instantiation was taking place.
>
> Now that we have object references, lets use those to ensure the object
> isn't deleted whilst we're working through the instantiation thus removing
> the need for recursive locks.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/virnwfilterobj.c | 13 +++++++++--
> src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++---------------
> 2 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 84ef7d1..ea1323d 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
> }
>
>
> +/**
> + * To avoid the need to have recursive locks as a result of how the
> + * virNWFilterInstantiateFilter processing works, this API will not
> + * lock the returned object, instead just increase the refcnt of the
> + * object to the caller to allow the caller to handle.
> + */
> virNWFilterObjPtr
> virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
> const char *filtername)
> {
> virNWFilterObjPtr obj;
>
> - if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) {
> + virObjectLock(nwfilters);
> + obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername);
> + virObjectUnlock(nwfilters);
> + if (!obj) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("referenced filter '%s' is missing"),
filtername);
> return NULL;
> @@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr
nwfilters,
> if (virNWFilterObjWantRemoved(obj)) {
> virReportError(VIR_ERR_NO_NWFILTER,
> _("Filter '%s' is in use."), filtername);
> - virNWFilterObjEndAPI(&obj);
> + virNWFilterObjUnref(obj);
> return NULL;
> }
So now an unlocked obj is returned. This feels wrong ... So for
instance: virNWFilterIncludeDefToRuleInst() calls
I wish this were fresher in my mind ;-)... But, IIRC the problem is that
without recursive locks and without being able to lock the hash table
(e.g. nwfilters list), then there was no way to find the filter by Name
because we'd already have the lock.
So my proposed solution is to skip locking altogether and use object
references in order to perform the searches.
virNWFilterObjListFindInstantiateFilter(). So it obtains ref'd
but
unlocked object. Then it calls virNWFilterObjGetDef() over it. Well, we
are reading without lock, so we might as well be accessing stale
pointer. For instance the following might happen:
1) threadA is in virNWFilterIncludeDefToRuleInst() and calls
virNWFilterObjGetDef(). It gets pointer to current definition of nwfilter.
2) threadB starts to update the definition of the object. Since the
object is not locked, nothing stops it from doing so. As part of the
process, current definition is freed.
3) threadA touches freed data.
But of course this points out the tragic flaw in the process. What if I
introduce something that uses pthread_mutex_trylock. For the "short
term" it's no different than pthread_mutex_lock since this is still a
recursive lock. The trylock would only be called from this path thus
further localizing the issue. Once converting from a recursive to a
normal lock, the code would need to handle the EBUSY return indicating
the lock was already taken. Since we don't know if it was taken by
ourselves or some other thread, then devise a mechanism to ensure no
other thread could attempt to lock whilst the processing for
virNWFilterObjListFindInstantiateFilter occurs.
Of course I'm typing this without having the entire algorithm in mind
right now. I'll focus some more on it and hopefully come up with
something so that the v2 would be "more correct".
Tks-
John