[PATCH v2 0/1] MR7668: wbemprox: Copy table in exec_select_view() instead of using per-table CS.
-- v2: wbemprox: Copy table in exec_select_view() instead of using per-table CS. https://gitlab.winehq.org/wine/wine/-/merge_requests/7668
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/wbemprox/builtin.c | 7 +-- dlls/wbemprox/class.c | 12 ++--- dlls/wbemprox/query.c | 31 ++++++----- dlls/wbemprox/services.c | 2 +- dlls/wbemprox/table.c | 93 ++++++++++++++++++++------------ dlls/wbemprox/wbemprox_private.h | 14 ++--- 6 files changed, 91 insertions(+), 68 deletions(-) diff --git a/dlls/wbemprox/builtin.c b/dlls/wbemprox/builtin.c index 646d93e3978..968d18061b3 100644 --- a/dlls/wbemprox/builtin.c +++ b/dlls/wbemprox/builtin.c @@ -4637,12 +4637,7 @@ void init_table_list( void ) { list_init( &tables[ns] ); for (i = 0; i < builtin_namespaces[ns].table_count; i++) - { - struct table *table = &builtin_namespaces[ns].tables[i]; - InitializeCriticalSectionEx( &table->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); - table->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": table.cs" ); - list_add_tail( &tables[ns], &table->entry ); - } + list_add_tail( &tables[ns], &builtin_namespaces[ns].tables[i].entry ); table_list[ns] = &tables[ns]; } } diff --git a/dlls/wbemprox/class.c b/dlls/wbemprox/class.c index 8e585c2f360..53822372bb7 100644 --- a/dlls/wbemprox/class.c +++ b/dlls/wbemprox/class.c @@ -114,7 +114,7 @@ static HRESULT WINAPI enum_class_object_Next( { struct enum_class_object *ec = impl_from_IEnumWbemClassObject( iface ); struct view *view = ec->query->view; - struct table *table; + const struct table *table; static int once = 0; HRESULT hr; ULONG i, j; @@ -219,7 +219,7 @@ HRESULT EnumWbemClassObject_create( struct query *query, LPVOID *ppObj ) return S_OK; } -static struct record *create_record( struct table *table ) +static struct record *create_record( const struct table *table ) { UINT i; struct record *record; @@ -536,7 +536,7 @@ static HRESULT WINAPI class_object_Next( struct class_object *obj = impl_from_IWbemClassObject( iface ); struct enum_class_object *iter = impl_from_IEnumWbemClassObject( obj->iter ); struct view *view = iter->query->view; - struct table *table = get_view_table( view, obj->index ); + const struct table *table = get_view_table( view, obj->index ); BSTR prop; HRESULT hr; UINT i; @@ -637,7 +637,7 @@ static BSTR get_body_text( const struct table *table, UINT row, UINT *len ) static BSTR get_object_text( const struct view *view, UINT index ) { UINT len, len_body, row = view->result[index]; - struct table *table = get_view_table( view, index ); + const struct table *table = get_view_table( view, index ); BSTR ret, body; len = ARRAY_SIZE( L"\ninstance of %s\n{%s\n};" ); @@ -686,7 +686,7 @@ static HRESULT WINAPI class_object_SpawnInstance( { struct class_object *co = impl_from_IWbemClassObject( iface ); struct enum_class_object *ec = impl_from_IEnumWbemClassObject( co->iter ); - struct table *table = get_view_table( ec->query->view, co->index ); + const struct table *table = get_view_table( ec->query->view, co->index ); IEnumWbemClassObject *iter; struct record *record; HRESULT hr; @@ -888,7 +888,7 @@ static HRESULT WINAPI class_object_GetMethod( { struct class_object *co = impl_from_IWbemClassObject( iface ); IWbemClassObject *in, *out; - struct table *table; + const struct table *table; unsigned int i; HRESULT hr; diff --git a/dlls/wbemprox/query.c b/dlls/wbemprox/query.c index a6599334f58..ef975990f0e 100644 --- a/dlls/wbemprox/query.c +++ b/dlls/wbemprox/query.c @@ -29,9 +29,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(wbemprox); -static HRESULT append_table( struct view *view, struct table *table ) +static HRESULT append_table( struct view *view, const struct table *table ) { - struct table **tmp; + const struct table **tmp; if (!(tmp = realloc( view->table, (view->table_count + 1) * sizeof(*tmp) ))) return E_OUTOFMEMORY; view->table = tmp; view->table[view->table_count++] = table; @@ -54,7 +54,7 @@ HRESULT create_view( enum view_type type, enum wbm_namespace ns, const WCHAR *pa case VIEW_TYPE_SELECT: { - struct table *table = find_table( ns, class ); + const struct table *table = find_table( ns, class ); HRESULT hr; if (table && (hr = append_table( view, table )) != S_OK) @@ -610,7 +610,7 @@ static HRESULT do_query( enum wbm_namespace ns, const WCHAR *str, struct query * } static HRESULT get_antecedent_table( enum wbm_namespace ns, const WCHAR *assocclass, const WCHAR *dependent, - struct table **table ) + const struct table **table ) { BSTR antecedent = NULL; struct path *path = NULL; @@ -657,7 +657,7 @@ static HRESULT exec_assoc_view( struct view *view ) { ULONG count; IWbemClassObject *obj; - struct table *table; + const struct table *table; VARIANT var; IEnumWbemClassObject_Next( iter, WBEM_INFINITE, 1, &obj, &count ); @@ -697,15 +697,20 @@ static HRESULT exec_select_view( struct view *view ) { UINT i, j = 0, len; enum fill_status status = FILL_STATUS_UNFILTERED; - struct table *table; + const struct table *table; if (!view->table_count) return S_OK; table = view->table[0]; if (table->fill) { - clear_table( table ); - status = table->fill( table, view->cond ); + struct table *table_copy; + + if (!(table_copy = get_table_writeable_copy( table ))) return E_OUTOFMEMORY; + clear_table( table_copy ); + status = table_copy->fill( table_copy, view->cond ); + release_table( table ); + table = view->table[0] = table_copy; } if (status == FILL_STATUS_FAILED) return WBEM_E_FAILED; if (!table->num_rows) return S_OK; @@ -869,7 +874,7 @@ static UINT count_key_columns( const struct table *table ) static BSTR build_relpath( const struct view *view, UINT table_index, UINT result_index, const WCHAR *name ) { BSTR class, proplist, ret = NULL; - struct table *table = view->table[table_index]; + const struct table *table = view->table[table_index]; UINT row = view->result[result_index]; UINT num_keys, len; @@ -1164,7 +1169,7 @@ static HRESULT map_view_index( const struct view *view, UINT index, UINT *table_ return S_OK; } -struct table *get_view_table( const struct view *view, UINT index ) +const struct table *get_view_table( const struct view *view, UINT index ) { switch (view->type) { @@ -1185,7 +1190,7 @@ HRESULT get_propval( const struct view *view, UINT index, const WCHAR *name, VAR { HRESULT hr; UINT column, row, table_index, result_index; - struct table *table; + const struct table *table; VARTYPE vartype; void *val_ptr = NULL; LONGLONG val; @@ -1382,7 +1387,7 @@ HRESULT put_propval( const struct view *view, UINT index, const WCHAR *name, VAR { HRESULT hr; UINT row, column, table_index, result_index; - struct table *table; + const struct table *table; LONGLONG val; if ((hr = map_view_index( view, index, &table_index, &result_index )) != S_OK) return hr; @@ -1412,7 +1417,7 @@ HRESULT get_properties( const struct view *view, UINT index, LONG flags, SAFEARR SAFEARRAY *sa; BSTR str; UINT i, table_index, result_index, count = 0; - struct table *table; + const struct table *table; HRESULT hr; LONG j = 0; diff --git a/dlls/wbemprox/services.c b/dlls/wbemprox/services.c index 3b55a534de9..29d2d576614 100644 --- a/dlls/wbemprox/services.c +++ b/dlls/wbemprox/services.c @@ -886,7 +886,7 @@ static HRESULT WINAPI wbem_services_ExecMethod( struct path *path; WCHAR *str; class_method *func; - struct table *table; + const struct table *table; HRESULT hr; TRACE( "%p, %s, %s, %#lx, %p, %p, %p, %p\n", iface, debugstr_w(strObjectPath), diff --git a/dlls/wbemprox/table.c b/dlls/wbemprox/table.c index 37aa546ad2b..c7b2911ad82 100644 --- a/dlls/wbemprox/table.c +++ b/dlls/wbemprox/table.c @@ -346,59 +346,52 @@ void free_table( struct table *table ) free( (WCHAR *)table->name ); free_columns( (struct column *)table->columns, table->num_cols ); free( table->data ); - - table->cs.DebugInfo->Spare[0] = 0; - DeleteCriticalSection( &table->cs ); free( table ); } -void release_table( struct table *table ) +void release_table( const struct table *ctable ) { - if (!--table->refs) - { - clear_table( table ); - if (table->flags & TABLE_FLAG_DYNAMIC) - { - EnterCriticalSection( &table_list_cs ); - list_remove( &table->entry ); - table->removed = TRUE; - LeaveCriticalSection( &table_list_cs ); + struct table *table = (struct table *)ctable; - LeaveCriticalSection( &table->cs ); - free_table( table ); - return; - } - } - LeaveCriticalSection( &table->cs ); + EnterCriticalSection( &table_list_cs ); + if (InterlockedDecrement( &table->refs )) goto done; + clear_table( table ); + if (!(table->flags & TABLE_FLAG_DYNAMIC)) goto done; + list_remove( &table->entry ); + table->removed = TRUE; + free_table( table ); +done: + LeaveCriticalSection( &table_list_cs ); } -struct table *grab_table( struct table *table ) +const struct table *grab_table( const struct table *table ) { - EnterCriticalSection( &table->cs ); - if (table->removed) - { - LeaveCriticalSection( &table->cs ); - return NULL; - } - table->refs++; + EnterCriticalSection( &table_list_cs ); + if (table->removed) table = NULL; + else InterlockedIncrement( (LONG *)&table->refs ); + LeaveCriticalSection( &table_list_cs ); return table; } -struct table *find_table( enum wbm_namespace ns, const WCHAR *name ) +const struct table *find_table( enum wbm_namespace ns, const WCHAR *name ) { + const struct table *ret = NULL; struct table *table; if (ns == WBEMPROX_NAMESPACE_LAST) return NULL; + EnterCriticalSection( &table_list_cs ); LIST_FOR_EACH_ENTRY( table, table_list[ns], struct table, entry ) { if (name && !wcsicmp( table->name, name )) { - TRACE("returning %p\n", table); - return grab_table( table ); + ret = grab_table( table ); + break; } } - return NULL; + LeaveCriticalSection( &table_list_cs ); + TRACE("returning %p\n", table); + return ret; } struct table *create_table( const WCHAR *name, UINT num_cols, const struct column *columns, @@ -419,11 +412,41 @@ struct table *create_table( const WCHAR *name, UINT num_cols, const struct colum table->refs = 0; table->removed = FALSE; list_init( &table->entry ); - InitializeCriticalSectionEx( &table->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); - table->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": table.cs"); return table; } +struct table *get_table_writeable_copy( const struct table *src ) +{ + struct table *table; + BYTE *data = NULL; + ULONG_PTR size; + unsigned int i; + + if (src->flags & TABLE_FLAG_DYNAMIC) + { + TRACE( "Table %p is dynamic.\n", src ); + return (struct table *)grab_table( src ); + } + + size = 0; + for (i = 0; i < src->num_cols; ++i) + size += get_column_size( src, i ); + size *= src->num_rows_allocated; + + if (size && src->data) + { + if (!(data = malloc( size ))) return NULL; + memcpy( data, src->data, size ); + } + if (!(table = create_table( src->name, src->num_cols, src->columns, src->num_rows, src->num_rows_allocated, + data, src->fill ))) + { + free( data ); + return NULL; + } + return (struct table *)grab_table( table ); +} + BOOL add_table( enum wbm_namespace ns, struct table *table ) { struct table *iter; @@ -449,7 +472,7 @@ BOOL add_table( enum wbm_namespace ns, struct table *table ) BSTR get_method_name( enum wbm_namespace ns, const WCHAR *class, UINT index ) { - struct table *table; + const struct table *table; UINT i, count = 0; BSTR ret; @@ -474,7 +497,7 @@ BSTR get_method_name( enum wbm_namespace ns, const WCHAR *class, UINT index ) WCHAR *get_first_key_property( enum wbm_namespace ns, const WCHAR *class ) { - struct table *table; + const struct table *table; WCHAR *ret = NULL; UINT i; diff --git a/dlls/wbemprox/wbemprox_private.h b/dlls/wbemprox/wbemprox_private.h index 5e585049aa6..164ba53f3a5 100644 --- a/dlls/wbemprox/wbemprox_private.h +++ b/dlls/wbemprox/wbemprox_private.h @@ -121,7 +121,6 @@ struct table UINT flags; struct list entry; LONG refs; - CRITICAL_SECTION cs; BOOL removed; }; @@ -154,7 +153,7 @@ struct record { UINT count; struct field *fields; - struct table *table; + const struct table *table; }; struct keyword @@ -179,7 +178,7 @@ struct view const struct property *proplist; /* SELECT query */ const struct expr *cond; UINT table_count; - struct table **table; + const struct table **table; UINT result_count; UINT *result; }; @@ -214,12 +213,13 @@ HRESULT create_view( enum view_type, enum wbm_namespace, const WCHAR *, const st const struct property *, const struct expr *, struct view ** ); void destroy_view( struct view * ); HRESULT execute_view( struct view * ); -struct table *get_view_table( const struct view *, UINT ); +const struct table *get_view_table( const struct view *, UINT ); void init_table_list( void ); enum wbm_namespace get_namespace_from_string( const WCHAR *namespace ); -struct table *find_table( enum wbm_namespace, const WCHAR * ); -struct table *grab_table( struct table * ); -void release_table( struct table * ); +const struct table *find_table( enum wbm_namespace, const WCHAR * ); +const struct table *grab_table( const struct table * ); +struct table *get_table_writeable_copy( const struct table * ); +void release_table( const struct table * ); struct table *create_table( const WCHAR *, UINT, const struct column *, UINT, UINT, BYTE *, enum fill_status (*)(struct table *, const struct expr *) ); BOOL add_table( enum wbm_namespace, struct table * ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7668
v2: - fix potential races in table list manipulation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99182
This is winehq bug 56859. I had been thinking about fixing this but in a different way. We shouldn't be holding a reference to the table (via query->view) from the enumerator object. We can instead create a list of class objects and copy data from the table view using the record structure. That also simplifies the class object implementation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99200
That probably means 'struct record' should stop referencing query->view? record->view->table is now also used class_object, not only enum_class_object. Are you already working on that? If not I could probably try to do something between the lines of that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99234
The table reference in struct record is only used to get the column index, which can be stored instead of the table reference. I had started on this and I can take it from here if you want. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99236
Yes, sure. Closing this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99240
This merge request was closed by Paul Gofman. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668
With that redesign are you planning to still get rid of those per table lock? Even if those locks won't be held outside of wbemprox method calls, I am afraid that still can deadlock on, e. g., parallel associated queries which access multiple tables. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99247
Right, the ASSOCIATORS OF queries are problematic. We'd get rid of the table lock for the enumerator but there might still be more than one result table. I have a patch that instanciates tables from static data and gets rid of the per table lock. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7668#note_99297
participants (3)
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)