Subject: Re: [PATCH v4 12/18] xen/pvcalls: implement poll command
To: Stefano Stabellini <sstabellini@kernel.org>
References: <alpine.DEB.2.10.1706151156350.12156@sstabellini-ThinkPad-X260>
 <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>
 <alpine.DEB.2.10.1706211331590.12819@sstabellini-ThinkPad-X260>
Cc: xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, jgross@suse.com,
        Stefano Stabellini <stefano@aporeto.com>
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <599ef1d4-cdf0-8710-ea96-cda9cc47a7c6@oracle.com>
Date: Wed, 21 Jun 2017 16:55:56 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.10.1706211331590.12819@sstabellini-ThinkPad-X260>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1120
Lines: 35


>>> +
>>> +	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.

-boris

>
>
>>> +	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;