From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761654AbdAKF04 (ORCPT ); Wed, 11 Jan 2017 00:26:56 -0500 Received: from mx2.suse.de ([195.135.220.15]:59920 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761598AbdAKF0z (ORCPT ); Wed, 11 Jan 2017 00:26:55 -0500 Subject: Re: [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses To: Boris Ostrovsky , linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org References: <20170106150544.10836-1-jgross@suse.com> <20170106150544.10836-4-jgross@suse.com> <14bc2980-fbb1-7a49-5308-3097a345e37d@oracle.com> From: Juergen Gross Message-ID: <2fddb09d-742a-e505-70ab-ec9815f93176@suse.com> Date: Wed, 11 Jan 2017 06:26:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <14bc2980-fbb1-7a49-5308-3097a345e37d@oracle.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/01/17 23:56, Boris Ostrovsky wrote: > > > >> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c >> index ebc768f..ebdfbee 100644 >> --- a/drivers/xen/xenbus/xenbus_xs.c >> +++ b/drivers/xen/xenbus/xenbus_xs.c > > >> - >> -static struct xs_handle xs_state; >> +/* >> + * Framework to protect suspend/resume handling against normal Xenstore >> + * message handling: >> + * During suspend/resume there must be no open transaction and no pending >> + * Xenstore request. >> + * New watch events happening in this time can be ignored by firing all watches >> + * after resume. >> + */ >> +/* Lock protecting enter/exit critical region. */ >> +static DEFINE_SPINLOCK(xs_state_lock); >> +/* Wait queue for all callers waiting for critical region to become usable. */ >> +static DECLARE_WAIT_QUEUE_HEAD(xs_state_enter_wq); >> +/* Wait queue for suspend handling waiting for critical region being empty. */ >> +static DECLARE_WAIT_QUEUE_HEAD(xs_state_exit_wq); >> +/* Number of users in critical region. */ >> +static unsigned int xs_state_users; >> +/* Suspend handler waiting or already active? */ >> +static int xs_suspend_active; > > I think these two should be declared next to xs_state _lock since they > are protected by it. Or maybe even put them into some sort of a state > struct. I think placing them near the lock and adding a comment is enough. >> + >> + >> +static bool test_reply(struct xb_req_data *req) >> +{ >> + if (req->state == xb_req_state_got_reply || !xenbus_ok()) >> + return true; >> + >> + /* Make sure to reread req->state each time. */ >> + cpu_relax(); > > I don't think I understand why this is needed. I need a compiler barrier. Otherwise the compiler read req->state only once outside the while loop. >> + >> + return false; >> +} >> + > > >> +static void xs_send(struct xb_req_data *req, struct xsd_sockmsg *msg) >> { >> - mutex_lock(&xs_state.transaction_mutex); >> - atomic_inc(&xs_state.transaction_count); >> - mutex_unlock(&xs_state.transaction_mutex); >> -} >> + bool notify; >> >> -static void transaction_end(void) >> -{ >> - if (atomic_dec_and_test(&xs_state.transaction_count)) >> - wake_up(&xs_state.transaction_wq); >> -} >> + req->msg = *msg; >> + req->err = 0; >> + req->state = xb_req_state_queued; >> + init_waitqueue_head(&req->wq); >> >> -static void transaction_suspend(void) >> -{ >> - mutex_lock(&xs_state.transaction_mutex); >> - wait_event(xs_state.transaction_wq, >> - atomic_read(&xs_state.transaction_count) == 0); >> -} >> + xs_request_enter(req); >> >> -static void transaction_resume(void) >> -{ >> - mutex_unlock(&xs_state.transaction_mutex); >> + req->msg.req_id = xs_request_id++; > > Is it safe to do this without a lock? You are right: I should move this to xs_request_enter() inside the lock. I think I'll let return xs_request_enter() the request id. >> + >> +int xenbus_dev_request_and_reply(struct xsd_sockmsg *msg, void *par) >> +{ >> + struct xb_req_data *req; >> + struct kvec *vec; >> + >> + req = kmalloc(sizeof(*req) + sizeof(*vec), GFP_KERNEL); > > Is there a reason why you are using different flags here? Yes. This function is always called in user context. No need to be more restrictive. >> @@ -263,11 +295,20 @@ static void *xs_talkv(struct xenbus_transaction t, >> unsigned int num_vecs, >> unsigned int *len) >> { >> + struct xb_req_data *req; >> struct xsd_sockmsg msg; >> void *ret = NULL; >> unsigned int i; >> int err; >> >> + req = kmalloc(sizeof(*req), GFP_NOIO | __GFP_HIGH); >> + if (!req) >> + return ERR_PTR(-ENOMEM); >> + >> + req->vec = iovec; >> + req->num_vecs = num_vecs; >> + req->cb = xs_wake_up; >> + >> msg.tx_id = t.id; >> msg.req_id = 0; > > Is this still needed? You are assigning it in xs_send(). Right. Can be removed. >> +static int xs_reboot_notify(struct notifier_block *nb, >> + unsigned long code, void *unused) >> { >> - struct xs_stored_msg *msg; > > > >> + struct xb_req_data *req; >> + >> + mutex_lock(&xb_write_mutex); >> + list_for_each_entry(req, &xs_reply_list, list) >> + wake_up(&req->wq); >> + list_for_each_entry(req, &xb_write_list, list) >> + wake_up(&req->wq); > > We are waking up waiters here but there is not guarantee that waiting > threads will have a chance to run, is there? You are right. But this isn't the point. We want to avoid blocking a reboot due to some needed thread waiting for xenstore. And this task is being accomplished here. Juergen