Re: [PATCH 1/6] xenbus: prepare data structures and parameter for xenwatch multithreading

[Dongli Zhang <dongli.zhang@oracle.com>]

Hi Boris,

On 09/17/2018 04:17 AM, Boris Ostrovsky wrote:
> 
> 
> On 9/14/18 3:34 AM, Dongli Zhang wrote:
> 
>> +
>> +struct mtwatch_info {
>> +    /*
>> +     * The mtwatch_domain is put on both a hash table and a list.
>> +     * domain_list is used to optimize xenbus_watch un-registration.
>> +     *
>> +     * The mtwatch_domain is removed from domain_hash (with state set
>> +     * to MTWATCH_DOMAIN_DOWN) when its refcnt is zero. However, it is
>> +     * left on domain_list until all events belong to such
>> +     * mtwatch_domain are processed in mtwatch_thread().
> 
> 
> Do we really need to keep mwatch_domain on both lists? Why is keeping it on,
> say, only the hash not sufficient?

In the state of the art xenbus, when a watch is unregistered (e.g.,
unregister_xenbus_watch()), we need to traverse the list 'watch_events' to
remove all inflight/pending events (for such watch) from 'watch_events'.

About this patch set, as each domain would have its own event list, we need to
traverse the list of each domain to remove the pending events for the watch to
be unregistered.

E.g.,
unregister_xenbus_watch()-->unregister_mtwatch()-->unregister_all_mtwatch() in
[PATCH 2/6] xenbus: implement the xenwatch multithreading framework.

To traverse a hash table is not as efficient as traversing a list. That's why a
domain is kept on both the hash table and list.

> 
>> +     *
>> +     * While there may exist two mtwatch_domain with the same domid on
>> +     * domain_list simultaneously,
> 
> 
> How is it possible to have two domids on the list at the same time? Don't you
> want to remove it (which IIUIC means moving it to the purge list) when domain is
> destroyed?

Here is one case (suppose the domid/frontend-id is 9):

1. Suppose the last pv driver device is removed from domid=9, and therefore the
reference count of per-domU xenwatch thread for domid=9 (which we call as old
thread below) should be removed. We remove it from hash table (it is left in the
list).

Here we cannot remove the domain from the list immediately because there might
be pending events being processed by the corresponding per-domU xenwatch thread.
If we remove it from the list while there is related watch being unregistered as
mentioned for last question, we may hit page fault when processing watch event.

2. Now the administrator attaches new pv device to domid=9 immediately and
therefore reference count is initially set to 1. The per-domU xenwatch thread
for domid=9 (called new thread) is created again. It is inserted to both the
hash table and list.

3. As the old thread for domid=9 might still be on the list, we would have two
threads for domid=9 (one old one to be removed and one newly inserted one to be
used by new pv devices).

Dongli Zhang

> 
> 
> -boris
> 
> 
>> +      *  all mtwatch_domain on hash_hash
>> +     * should have unique domid.
>> +     */
>> +    spinlock_t domain_lock;
>> +    struct hlist_head domain_hash[MTWATCH_HASH_SIZE];
>> +    struct list_head domain_list;
>> +
>> +    /*
>> +     * When a per-domU kthread is going to be destroyed, it is put
>> +     * on the purge_list, and will be flushed by purge_work later.
>> +     */
>> +    struct work_struct purge_work;
>> +    spinlock_t purge_lock;
>> +    struct list_head purge_list;
>> +};
> 
>