On 10/19/2017 10:07 AM, Erik Skultety wrote:
> On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
>> Rather than a forward linked list, let's use the ObjectRWLockable object
>> in order to manage the data.
>>
>> Requires numerous changes from List to Object management similar to
>> many other drivers/vir*obj.c modules
>>
>
> This patch should be split in two. One that converts it to RWLockable, the
> other using a hash table instead of a linked list.
>
> [...]
Personally, I don't recall during various conversions I've done to having:
virObjectLockable parent;
size_t count;
virInterfaceObjPtr *objs;
first, then converting to count/objs to (a) hash table(s). Although
looking through history I see Michal had the networkobj code take a long
winding path to get there, so it's not impossible - just IMO pointless.
I think whenever I've converted from count/obs to a hash table with a
'real object', that the locking was done a the same time.
As I recall, usually it's been converting from :
size_t count;
virInterfaceObjPtr *objs;
to:
virObjectLockable parent;
/* name string -> virInterfaceObj mapping
* for O(1), lockless lookup-by-name */
virHashTable *objsName;
then more recently the conversion from virObjectLockable to
virObjectRWLockable
Since this patch is taking @objs and converting to a hash table - it's
avoiding the Lockable and going straight to RWLockable.
>>
>> @@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>> {
>> virInterfaceObjPtr obj;
>>
>> - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
>> + virObjectRWLockWrite(interfaces);
>> + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
>> virInterfaceDefFree(obj->def);
>> obj->def = def;
>> + virObjectRWUnlock(interfaces);
>>
>> return obj;
>> }
>> @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>> if (!(obj = virInterfaceObjNew()))
>> return NULL;
>>
>> - if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
>> - interfaces->count, obj) < 0) {
>> - virInterfaceObjEndAPI(&obj);
>> - return NULL;
>> - }
>> + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
>> + goto error;
>> + virObjectRef(obj);
>> +
>> obj->def = def;
>> - return virObjectRef(obj);
>> + virObjectRWUnlock(interfaces);
>> +
>> + return obj;
>> +
>> + error:
>> + virInterfaceObjEndAPI(&obj);
>> + virObjectRWUnlock(interfaces);
>> + return NULL;
>> }
>
> ^This could be tweaked even more:
>
Sure, but in doing so eagle eyes now spots a problem in his own code...
> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
> index cf3626def..941893fc5 100644
> --- a/src/conf/virinterfaceobj.c
> +++ b/src/conf/virinterfaceobj.c
> @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
> virObjectRWLockWrite(interfaces);
> if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
> virInterfaceDefFree(obj->def);
> - obj->def = def;
> - virObjectRWUnlock(interfaces);
> + } else {
> + if (!(obj = virInterfaceObjNew()))
> + return NULL;
Leaving with RWLock, <sigh>
>
> - return obj;
> + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> + goto error;
> + virObjectRef(obj);
> }
>
> - if (!(obj = virInterfaceObjNew()))
> - return NULL;
> -
> - if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> - goto error;
> - virObjectRef(obj);
> -
> obj->def = def;
> virObjectRWUnlock(interfaces);
>
>>
>>
>> @@ -277,20 +366,37 @@ void
>> virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
>> virInterfaceObjPtr obj)
>> {
>> - size_t i;
>
> if (!obj)
> return;
>
OK - I think at some point in time I figured it wasn't possible, but
adding it doesn't hurt. It's there in my branch.
> I didn't bother to check whether it could happen (it's used in test driver only
> anyway), but just in case there's an early cleanup exit path like it was in my
> recent nodedev code which was missing this very check too which would have made
> it fail miserably in such scenario.
>
> With the patch split in 2 introducing 2 distinct changes + the NULL check:
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
Hopefully you reconsider the desire for 2 patches...
>
>
> PS: I'm also sad, that we have two backends here just like we have in nodedev
> with one of them being essentially useless (just like in nodedev) we have this
> 'conf' generic code (just like in nodedev), yet in this case it's only used in
> the test driver. I'd very much appreciate if those backends could be adjusted
> in a way where we could make use of these functions. I can also imagine a
> cooperation of the udev backend with the nodedev driver where we have an active
> connection to the monitor, thus reacting to all events realtime, instead of
> defining a bunch of filters and then letting udev re-enumerate the list of
> interfaces each and every time (and by saying that I'm also aware that udev is
> actually the useless backend here).
>
> Erik
>
Yeah - the driver code here is quite different/strange and could
possibly use a bit more convergence. I feel too battered and bruised
over this convergence right now though ;-).... Besides the differences
between the two really kind of are a bit scary w/r/t needing to call
some sort of netcf* or udev* interface to get the answers.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
