lkml.org 
[lkml]   [2018]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/6] xenbus: implement the xenwatch multithreading framework
From
Date


On 9/14/18 3:34 AM, Dongli Zhang wrote:
>
> +
> +/* Running in the context of default xenwatch kthread. */
> +void mtwatch_create_domain(domid_t domid)
> +{
> + struct mtwatch_domain *domain;
> +
> + if (!domid) {
> + pr_err("Default xenwatch thread is for dom0\n");
> + return;
> + }
> +
> + spin_lock(&mtwatch_info->domain_lock);
> +
> + domain = mtwatch_find_domain(domid);
> + if (domain) {
> + atomic_inc(&domain->refcnt);
> + spin_unlock(&mtwatch_info->domain_lock);
> + return;
> + }
> +
> + domain = kzalloc(sizeof(*domain), GFP_ATOMIC);

Is there a reason (besides this being done under spinlock) for using
GFP_ATOMIC? If domain_lock is the only reason I'd probably drop the lock
and do GFP_KERNEL.


> + if (!domain) {
> + spin_unlock(&mtwatch_info->domain_lock);
> + pr_err("Failed to allocate memory for mtwatch thread %d\n",
> + domid);
> + return;

This needs to return an error code, I think. Or do you want to fall back
to shared xenwatch thread?


> + }
> +
> + domain->domid = domid;
> + atomic_set(&domain->refcnt, 1);
> + mutex_init(&domain->domain_mutex);
> + INIT_LIST_HEAD(&domain->purge_node);
> +
> + init_waitqueue_head(&domain->events_wq);
> + spin_lock_init(&domain->events_lock);
> + INIT_LIST_HEAD(&domain->events);
> +
> + list_add_tail_rcu(&domain->list_node, &mtwatch_info->domain_list);
> +
> + hlist_add_head_rcu(&domain->hash_node,
> + &mtwatch_info->domain_hash[MTWATCH_HASH(domid)]);
> +
> + spin_unlock(&mtwatch_info->domain_lock);
> +
> + domain->task = kthread_run(mtwatch_thread, domain,
> + "xen-mtwatch-%d", domid);
> +
> + if (!domain->task) {
> + pr_err("mtwatch kthread creation is failed\n");
> + domain->state = MTWATCH_DOMAIN_DOWN;


Why not clean up right here?

> +
> + return;
> + }
> +
> + domain->state = MTWATCH_DOMAIN_UP;
> +}
> +


> +
> void unregister_xenbus_watch(struct xenbus_watch *watch)
> {
> struct xs_watch_event *event, *tmp;
> @@ -831,6 +1100,9 @@ void unregister_xenbus_watch(struct xenbus_watch *watch)
>
> if (current->pid != xenwatch_pid)
> mutex_unlock(&xenwatch_mutex);
> +
> + if (xen_mtwatch && watch->get_domid)
> + unregister_mtwatch(watch);


I may not be understanding the logic flow here, but if we successfully
removed/unregistered/purged the watch from mtwatch lists, do we still
need to try removing it from watch_events list below?


-boris

\
 
 \ /
  Last update: 2018-09-16 23:20    [W:0.188 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site