Message-ID: <7ae3b919-1e67-e4a0-22ac-92f8f91d4686@oracle.com>
Date:   Wed, 26 Sep 2018 10:57:48 +0800
From:   Dongli Zhang <dongli.zhang@...cle.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org
Cc:     jgross@...e.com, wei.liu2@...rix.com, konrad.wilk@...cle.com,
        srinivas.eeda@...cle.com, paul.durrant@...rix.com,
        roger.pau@...rix.com
Subject: Re: [Xen-devel] [PATCH 1/6] xenbus: prepare data structures and
 parameter for xenwatch multithreading

Hi Boris,

On 09/26/2018 04:19 AM, Boris Ostrovsky wrote:
> On 9/25/18 1:14 AM, Dongli Zhang wrote:
>>
>> So far we have: (1) domain hash table, (2) domain list (where duplicate entries
>> may exist) and (3) purge list.
>>
>> Can I assume you would like to discard the domain list and only keep domain hash
>> table and purge list?
> 
> Yes, that's what I was thinking.
> 
>>
>> The purpose of the domain list is to facilitate the unregister_xenbus_watch() to
>> scan the pending events for all otherend id. Should we remove it? Xen hypervisor
>> used both a hash table and a list to manage struct domain.
> 
> 
> Your concern is that searching for a pending event in the hash is not
> especially efficient. Since you are hashing on domain_id, then
> unregister_single_mtwatch() should be pretty fast if searching in the
> hash table (in fact, it's faster than traversing the domain list).
> unregister_all_mtwatch() will indeed take longer since you will be
> checking empty buckets. Right?
> 
> How often do you expect will we call unregister_all_mtwatch() compared
> to unregister_single_mtwatch()?

In the patch set v1, unregister_all_mtwatch() is indeed never called.


The watch registered globally seems never (let's use "rarely") unregistered.
E.g., the be_watch (at node 'backend') is registered during initialization and
will be used during the lifecycle of a backend domain (dom0 or driver domain).

I agree that unregister_all_mtwatch() is not called so far and will not be used
very often in the future. The performance overhead is trivial.

Therefore, I would discard the domain list. Only 'domain hash' and 'purge list'
will be used.

> 
> 
>>
>>
>> About the duplicate entries in the domain list, can we change the flow like below:
>>
>> 1. Suppose the thread status is DOWN. To avoid having duplicate entries on the
>> domain list, instead of keeping the deprecated thread on domain list (until all
>> its events get processed), we move it to the purge list immediately.
>>
>> We will tag this deprecated thread so that the purge list would not purge it
>> unless all events belong to such thread get processed. We cannot purge it
>> immediately because the worker thread (to purge the purge list) would hang if
>> the deprecated thread is already stuck.
>>
>> In this flow, we may have duplicate entries on purge list, but not domain list.
>>
>> 2. During unregister_xenbus_watch(), we need to scan both the domain list and
>> purge list to remove pending events for the watch. In previous design, we only
>> scan domain lit.
>>
>>
>> One option is we allow the deprecated thread to resurrect and we would not move
>> the thread to purge list immediately when the thread is deprecated.
>>
>> Suppose when thread for domid=9 is needed, we would not create new one if the
>> deprecated one for domid=9 is still in domain list. Instead, we would resurrect
>> it, change its status and reuse it again. In this way, we would not have
>> duplicate entries on the domain list.
>>
>> I like the 1st option. I do not like to resurrect a deprecated thread again.
>> Would you please let me know how you think about it?
> 
> 
> Yes, I also think (1) is better --- you are walking the whole purge list
> anyway.

I will take (1).

when unregister_all_mtwatch(): scan both 'hash table' and 'purge list'.

The entries on the purge list are classified (tagged) as (1) can be purged
immediately, and (2) not purge until it completes all pending events.



Thank you very much!

Dongli Zhang

> 
> 
> -boris
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@...ts.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

Powered by blists - more mailing lists