From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938238AbdAKPaf convert rfc822-to-8bit (ORCPT ); Wed, 11 Jan 2017 10:30:35 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:33198 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751563AbdAKP3B (ORCPT ); Wed, 11 Jan 2017 10:29:01 -0500 Subject: Re: [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses To: Juergen Gross , 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> <2fddb09d-742a-e505-70ab-ec9815f93176@suse.com> From: Boris Ostrovsky Message-ID: <4a9f657b-1cf1-94ff-daf8-c928b644e044@oracle.com> Date: Wed, 11 Jan 2017 10:29:01 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <2fddb09d-742a-e505-70ab-ec9815f93176@suse.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8BIT X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >>> + >>> + >>> +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. Then barrier() looks the right primitive to use here. cpu_relax(), while doing what you want, is intended for other purposes. > >>> + >>> + 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. Then please move xs_request_id's declaration close to xs_state_lock's declaration (just like you are going to move the two other state variables) > >>> +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. I think it's worth adding a comment mentioning this. -boris From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses Date: Wed, 11 Jan 2017 10:29:01 -0500 Message-ID: <4a9f657b-1cf1-94ff-daf8-c928b644e044@oracle.com> References: <20170106150544.10836-1-jgross@suse.com> <20170106150544.10836-4-jgross@suse.com> <14bc2980-fbb1-7a49-5308-3097a345e37d@oracle.com> <2fddb09d-742a-e505-70ab-ec9815f93176@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cRKpb-000685-6f for xen-devel@lists.xenproject.org; Wed, 11 Jan 2017 15:28:55 +0000 In-Reply-To: <2fddb09d-742a-e505-70ab-ec9815f93176@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Juergen Gross , linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org Cj4+PiArCj4+PiArCj4+PiArc3RhdGljIGJvb2wgdGVzdF9yZXBseShzdHJ1Y3QgeGJfcmVxX2Rh dGEgKnJlcSkKPj4+ICt7Cj4+PiArCWlmIChyZXEtPnN0YXRlID09IHhiX3JlcV9zdGF0ZV9nb3Rf cmVwbHkgfHwgIXhlbmJ1c19vaygpKQo+Pj4gKwkJcmV0dXJuIHRydWU7Cj4+PiArCj4+PiArCS8q IE1ha2Ugc3VyZSB0byByZXJlYWQgcmVxLT5zdGF0ZSBlYWNoIHRpbWUuICovCj4+PiArCWNwdV9y ZWxheCgpOwo+PiBJIGRvbid0IHRoaW5rIEkgdW5kZXJzdGFuZCB3aHkgdGhpcyBpcyBuZWVkZWQu Cj4gSSBuZWVkIGEgY29tcGlsZXIgYmFycmllci4gT3RoZXJ3aXNlIHRoZSBjb21waWxlciByZWFk IHJlcS0+c3RhdGUgb25seQo+IG9uY2Ugb3V0c2lkZSB0aGUgd2hpbGUgbG9vcC4KCgpUaGVuIGJh cnJpZXIoKSBsb29rcyB0aGUgcmlnaHQgcHJpbWl0aXZlIHRvIHVzZSBoZXJlLiBjcHVfcmVsYXgo KSwgd2hpbGUKZG9pbmcgd2hhdCB5b3Ugd2FudCwgaXMgaW50ZW5kZWQgZm9yIG90aGVyIHB1cnBv c2VzLgoKCj4KPj4+ICsKPj4+ICsJcmV0dXJuIGZhbHNlOwo+Pj4gK30KPj4+ICsKPj4KPj4+ICtz dGF0aWMgdm9pZCB4c19zZW5kKHN0cnVjdCB4Yl9yZXFfZGF0YSAqcmVxLCBzdHJ1Y3QgeHNkX3Nv Y2ttc2cgKm1zZykKPj4+ICB7Cj4+PiAtCW11dGV4X2xvY2soJnhzX3N0YXRlLnRyYW5zYWN0aW9u X211dGV4KTsKPj4+IC0JYXRvbWljX2luYygmeHNfc3RhdGUudHJhbnNhY3Rpb25fY291bnQpOwo+ Pj4gLQltdXRleF91bmxvY2soJnhzX3N0YXRlLnRyYW5zYWN0aW9uX211dGV4KTsKPj4+IC19Cj4+ PiArCWJvb2wgbm90aWZ5Owo+Pj4gIAo+Pj4gLXN0YXRpYyB2b2lkIHRyYW5zYWN0aW9uX2VuZCh2 b2lkKQo+Pj4gLXsKPj4+IC0JaWYgKGF0b21pY19kZWNfYW5kX3Rlc3QoJnhzX3N0YXRlLnRyYW5z YWN0aW9uX2NvdW50KSkKPj4+IC0JCXdha2VfdXAoJnhzX3N0YXRlLnRyYW5zYWN0aW9uX3dxKTsK Pj4+IC19Cj4+PiArCXJlcS0+bXNnID0gKm1zZzsKPj4+ICsJcmVxLT5lcnIgPSAwOwo+Pj4gKwly ZXEtPnN0YXRlID0geGJfcmVxX3N0YXRlX3F1ZXVlZDsKPj4+ICsJaW5pdF93YWl0cXVldWVfaGVh ZCgmcmVxLT53cSk7Cj4+PiAgCj4+PiAtc3RhdGljIHZvaWQgdHJhbnNhY3Rpb25fc3VzcGVuZCh2 b2lkKQo+Pj4gLXsKPj4+IC0JbXV0ZXhfbG9jaygmeHNfc3RhdGUudHJhbnNhY3Rpb25fbXV0ZXgp Owo+Pj4gLQl3YWl0X2V2ZW50KHhzX3N0YXRlLnRyYW5zYWN0aW9uX3dxLAo+Pj4gLQkJICAgYXRv bWljX3JlYWQoJnhzX3N0YXRlLnRyYW5zYWN0aW9uX2NvdW50KSA9PSAwKTsKPj4+IC19Cj4+PiAr CXhzX3JlcXVlc3RfZW50ZXIocmVxKTsKPj4+ICAKPj4+IC1zdGF0aWMgdm9pZCB0cmFuc2FjdGlv bl9yZXN1bWUodm9pZCkKPj4+IC17Cj4+PiAtCW11dGV4X3VubG9jaygmeHNfc3RhdGUudHJhbnNh Y3Rpb25fbXV0ZXgpOwo+Pj4gKwlyZXEtPm1zZy5yZXFfaWQgPSB4c19yZXF1ZXN0X2lkKys7Cj4+ IElzIGl0IHNhZmUgdG8gZG8gdGhpcyB3aXRob3V0IGEgbG9jaz8KPiBZb3UgYXJlIHJpZ2h0OiBJ IHNob3VsZCBtb3ZlIHRoaXMgdG8geHNfcmVxdWVzdF9lbnRlcigpIGluc2lkZSB0aGUKPiBsb2Nr LiBJIHRoaW5rIEknbGwgbGV0IHJldHVybiB4c19yZXF1ZXN0X2VudGVyKCkgdGhlIHJlcXVlc3Qg aWQuCgoKVGhlbiBwbGVhc2UgbW92ZSB4c19yZXF1ZXN0X2lkJ3MgZGVjbGFyYXRpb24gY2xvc2Ug dG8geHNfc3RhdGVfbG9jaydzCmRlY2xhcmF0aW9uIChqdXN0IGxpa2UgeW91IGFyZSBnb2luZyB0 byBtb3ZlIHRoZSB0d28gb3RoZXIgc3RhdGUgdmFyaWFibGVzKQoKCj4KPj4+ICtzdGF0aWMgaW50 IHhzX3JlYm9vdF9ub3RpZnkoc3RydWN0IG5vdGlmaWVyX2Jsb2NrICpuYiwKPj4+ICsJCQkgICAg dW5zaWduZWQgbG9uZyBjb2RlLCB2b2lkICp1bnVzZWQpCj4+PiAgewo+Pj4gLQlzdHJ1Y3QgeHNf c3RvcmVkX21zZyAqbXNnOwo+Pgo+Pgo+Pj4gKwlzdHJ1Y3QgeGJfcmVxX2RhdGEgKnJlcTsKPj4+ ICsKPj4+ICsJbXV0ZXhfbG9jaygmeGJfd3JpdGVfbXV0ZXgpOwo+Pj4gKwlsaXN0X2Zvcl9lYWNo X2VudHJ5KHJlcSwgJnhzX3JlcGx5X2xpc3QsIGxpc3QpCj4+PiArCQl3YWtlX3VwKCZyZXEtPndx KTsKPj4+ICsJbGlzdF9mb3JfZWFjaF9lbnRyeShyZXEsICZ4Yl93cml0ZV9saXN0LCBsaXN0KQo+ Pj4gKwkJd2FrZV91cCgmcmVxLT53cSk7Cj4+IFdlIGFyZSB3YWtpbmcgdXAgd2FpdGVycyBoZXJl IGJ1dCB0aGVyZSBpcyBub3QgZ3VhcmFudGVlIHRoYXQgd2FpdGluZwo+PiB0aHJlYWRzIHdpbGwg aGF2ZSBhIGNoYW5jZSB0byBydW4sIGlzIHRoZXJlPwo+IFlvdSBhcmUgcmlnaHQuIEJ1dCB0aGlz IGlzbid0IHRoZSBwb2ludC4gV2Ugd2FudCB0byBhdm9pZCBibG9ja2luZyBhCj4gcmVib290IGR1 ZSB0byBzb21lIG5lZWRlZCB0aHJlYWQgd2FpdGluZyBmb3IgeGVuc3RvcmUuIEFuZCB0aGlzIHRh c2sKPiBpcyBiZWluZyBhY2NvbXBsaXNoZWQgaGVyZS4KCgpJIHRoaW5rIGl0J3Mgd29ydGggYWRk aW5nIGEgY29tbWVudCBtZW50aW9uaW5nIHRoaXMuCgotYm9yaXMKCgpfX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpYZW4tZGV2ZWwgbWFpbGluZyBsaXN0Clhl bi1kZXZlbEBsaXN0cy54ZW4ub3JnCmh0dHBzOi8vbGlzdHMueGVuLm9yZy94ZW4tZGV2ZWwK