Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752020AbdFUVt0 (ORCPT ); Wed, 21 Jun 2017 17:49:26 -0400 Received: from mail.kernel.org ([198.145.29.99]:41354 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751108AbdFUVtZ (ORCPT ); Wed, 21 Jun 2017 17:49:25 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BFD6A217C4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=sstabellini@kernel.org Date: Wed, 21 Jun 2017 14:49:20 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Boris Ostrovsky cc: Stefano Stabellini , xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, jgross@suse.com, Stefano Stabellini Subject: Re: [PATCH v4 12/18] xen/pvcalls: implement poll command In-Reply-To: <599ef1d4-cdf0-8710-ea96-cda9cc47a7c6@oracle.com> Message-ID: References: <1497553787-3709-1-git-send-email-sstabellini@kernel.org> <1497553787-3709-12-git-send-email-sstabellini@kernel.org> <15ea89f6-d46e-2495-1b52-d7290b47c478@oracle.com> <599ef1d4-cdf0-8710-ea96-cda9cc47a7c6@oracle.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1551 Lines: 42 On Wed, 21 Jun 2017, Boris Ostrovsky wrote: > >>> + > >>> + mappass->reqcopy = *req; > >>> + icsk = inet_csk(mappass->sock->sk); > >>> + queue = &icsk->icsk_accept_queue; > >>> + spin_lock(&queue->rskq_lock); > >>> + data = queue->rskq_accept_head != NULL; > >>> + spin_unlock(&queue->rskq_lock); > >> What is the purpose of the queue lock here? > > It is only there to protect accesses to rskq_accept_head. Functions that > > change rskq_accept_head take this lock, see for example > > net/ipv4/inet_connection_sock.c:inet_csk_reqsk_queue_add. I'll add an > > in-code comment. > > I am not sure I follow. You are not changing rskq_accept_head, you are > simply reading it under the lock. It may be set by others to NULL as > soon as you drop the lock, at which point 'data' test below will be > obsolete. > > In inet_csk_reqsk_queue_add() it is read and then, based on read result, > is written with a value so a lock is indeed need there. I think you are right. The only thing is that without the lock we might read a transitory value as the rskq_accept_head reads/writes are not guaranteed to be atomic. However, I don't think we care about it, since this is just a != NULL test and, as you wrote, the result could be obsolete immediately after. I'll drop the lock. > > > > > >>> + if (data) { > >>> + mappass->reqcopy.cmd = 0; > >>> + ret = 0; > >>> + goto out; > >>> + } > >>> + spin_unlock_irqrestore(&mappass->copy_lock, flags); > >>> + > >>> + /* Tell the caller we don't need to send back a notification yet */ > >>> + return -1; >