Received: by 10.223.185.116 with SMTP id b49csp2393486wrg; Mon, 12 Feb 2018 08:52:05 -0800 (PST) X-Google-Smtp-Source: AH8x224P9qJBTomxODIfhJQWQ/BTV4nw/jZFT86AhBlEkEyUuPTnyW7BZPz2SCOAW7h8ya2rt8LU X-Received: by 2002:a17:902:d83:: with SMTP id 3-v6mr11247352plv.82.1518454325580; Mon, 12 Feb 2018 08:52:05 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518454325; cv=none; d=google.com; s=arc-20160816; b=V0m1+ttmasr7prKfbNal44zvwUXwE4RISKeF3zj44hOLI3UeTL62rKIeDn6Qnh17Ef S1cgMhAiycx6vahriUUxor1QzgycqTogRQQIy+KIRCp065EqR8I9TE+qb1AZk1OQ9Zu3 8h9o2WBWIwR9jBL3gtk1VIsJ7CcqE9+rr5QUmLh95Yq/d3RDJG43XNW0UxUFFwUn0dx8 t1CjRcyuoP2SUNoiFfiY2gxJCPcrEvT5OzCpGREcq50rQyPpKZMO9Z7TC02lms2exAqA W0dbICjRqsaG0RACPfNbWqbC6iOL87e/KQMKKXhhHI17XOtB1t9bhj/LxBLxhOkvjcbf kIAA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=dGqHRI/Al7Ap7FC6c3Tte0Gzxgkt8RL1XzNPlY3KRSU=; b=A4m4zsI21Sru0ZgAMZVP4GWikGj/w8i4OsCluqAyanPlCXWrhSuf7jBhWc75qCf4AH 7FGEi4UxB4PG4TR+1li1PlhF8m5Ervfe2u1fET+zPcLtU5cJOHB0HSRC8qT5ZU8lRqpM W3MlFet4PQJWNJ9fzZvUcSSX9Is9InKBgBYxaKbCdLHmIXcWFzYeG79rSTMkUoaeVdaf f+qoAzj3cFzxapJaqS7X745OnnKWw+qbECmXLVEMUwWrSULZExIAoyq8kLHbDuDkcodv F9lmZOCMcT2LaMWdmso9bJKjLnzdKTxNegdZYuhwjD9nd8H3UlvsaP5snjRY/Hy340px 8jZQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w12si46781pgr.211.2018.02.12.08.51.50; Mon, 12 Feb 2018 08:52:05 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935180AbeBLNO7 (ORCPT + 99 others); Mon, 12 Feb 2018 08:14:59 -0500 Received: from mx2.suse.de ([195.135.220.15]:39467 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933428AbeBLNO6 (ORCPT ); Mon, 12 Feb 2018 08:14:58 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 3EF2CAB02; Mon, 12 Feb 2018 13:14:57 +0000 (UTC) Subject: Re: [PATCH 1/2] pvcalls-front: introduce a per sock_mapping refcount To: Stefano Stabellini , boris.ostrovsky@oracle.com Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, Stefano Stabellini References: <1517871107-15349-1-git-send-email-sstabellini@kernel.org> From: Juergen Gross Message-ID: Date: Mon, 12 Feb 2018 14:14:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1517871107-15349-1-git-send-email-sstabellini@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Language: de-DE Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/02/18 23:51, Stefano Stabellini wrote: > Introduce a per sock_mapping refcount, in addition to the existing > global refcount. Thanks to the sock_mapping refcount, we can safely wait > for it to be 1 in pvcalls_front_release before freeing an active socket, > instead of waiting for the global refcount to be 1. > > Signed-off-by: Stefano Stabellini > --- > drivers/xen/pvcalls-front.c | 190 ++++++++++++++++++-------------------------- > 1 file changed, 78 insertions(+), 112 deletions(-) > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c > index 4c789e6..164d3ad 100644 > --- a/drivers/xen/pvcalls-front.c > +++ b/drivers/xen/pvcalls-front.c > @@ -60,6 +60,7 @@ struct sock_mapping { > bool active_socket; > struct list_head list; > struct socket *sock; > + atomic_t refcount; > union { > struct { > int irq; > @@ -93,6 +94,33 @@ struct sock_mapping { > }; > }; > > +static inline struct sock_mapping *pvcalls_enter_sock(struct socket *sock) > +{ > + struct sock_mapping *map = NULL; > + > + if (!pvcalls_front_dev || &pvcalls_front_dev->dev == NULL) Did you mean: if (!pvcalls_front_dev || !pvcalls_front_dev->dev) > + return ERR_PTR(-ENOTCONN); > + > + pvcalls_enter(); > + map = (struct sock_mapping *) sock->sk->sk_send_head; Style: no blank after the cast, please (multiple times). > + if (map == NULL) { > + pvcalls_exit() > + return ERR_PTR(-ENOTSOCK); > + } > + > + atomic_inc(&map->refcount); > + return map; > +} > + > +static inline void pvcalls_exit_sock(struct socket *sock) > +{ > + struct sock_mapping *map = NULL; > + > + map = (struct sock_mapping *) sock->sk->sk_send_head; > + atomic_dec(&map->refcount); > + pvcalls_exit(); > +} > + > static inline int get_request(struct pvcalls_bedata *bedata, int *req_id) > { > *req_id = bedata->ring.req_prod_pvt & (RING_SIZE(&bedata->ring) - 1); > @@ -369,31 +397,23 @@ int pvcalls_front_connect(struct socket *sock, struct sockaddr *addr, > if (addr->sa_family != AF_INET || sock->type != SOCK_STREAM) > return -EOPNOTSUPP; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > - return -ENOTCONN; > - } > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > + return PTR_ERR(map); > > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *)sock->sk->sk_send_head; > - if (!map) { > - pvcalls_exit(); > - return -ENOTSOCK; > - } > - > spin_lock(&bedata->socket_lock); > ret = get_request(bedata, &req_id); > if (ret < 0) { > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > ret = create_active(map, &evtchn); > if (ret < 0) { > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > > @@ -423,7 +443,7 @@ int pvcalls_front_connect(struct socket *sock, struct sockaddr *addr, > smp_rmb(); > ret = bedata->rsp[req_id].ret; > bedata->rsp[req_id].req_id = PVCALLS_INVALID_ID; > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > > @@ -488,23 +508,15 @@ int pvcalls_front_sendmsg(struct socket *sock, struct msghdr *msg, > if (flags & (MSG_CONFIRM|MSG_DONTROUTE|MSG_EOR|MSG_OOB)) > return -EOPNOTSUPP; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > - return -ENOTCONN; > - } > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > + return PTR_ERR(map); > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (!map) { > - pvcalls_exit(); > - return -ENOTSOCK; > - } > - > mutex_lock(&map->active.out_mutex); > if ((flags & MSG_DONTWAIT) && !pvcalls_front_write_todo(map)) { > mutex_unlock(&map->active.out_mutex); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EAGAIN; > } > if (len > INT_MAX) > @@ -526,7 +538,7 @@ int pvcalls_front_sendmsg(struct socket *sock, struct msghdr *msg, > tot_sent = sent; > > mutex_unlock(&map->active.out_mutex); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return tot_sent; > } > > @@ -591,19 +603,11 @@ int pvcalls_front_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > if (flags & (MSG_CMSG_CLOEXEC|MSG_ERRQUEUE|MSG_OOB|MSG_TRUNC)) > return -EOPNOTSUPP; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > - return -ENOTCONN; > - } > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > + return PTR_ERR(map); > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (!map) { > - pvcalls_exit(); > - return -ENOTSOCK; > - } > - > mutex_lock(&map->active.in_mutex); > if (len > XEN_FLEX_RING_SIZE(PVCALLS_RING_ORDER)) > len = XEN_FLEX_RING_SIZE(PVCALLS_RING_ORDER); > @@ -623,7 +627,7 @@ int pvcalls_front_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > ret = 0; > > mutex_unlock(&map->active.in_mutex); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > > @@ -637,24 +641,16 @@ int pvcalls_front_bind(struct socket *sock, struct sockaddr *addr, int addr_len) > if (addr->sa_family != AF_INET || sock->type != SOCK_STREAM) > return -EOPNOTSUPP; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > - return -ENOTCONN; > - } > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > + return PTR_ERR(map); > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (map == NULL) { > - pvcalls_exit(); > - return -ENOTSOCK; > - } > - > spin_lock(&bedata->socket_lock); > ret = get_request(bedata, &req_id); > if (ret < 0) { > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > req = RING_GET_REQUEST(&bedata->ring, req_id); > @@ -684,7 +680,7 @@ int pvcalls_front_bind(struct socket *sock, struct sockaddr *addr, int addr_len) > bedata->rsp[req_id].req_id = PVCALLS_INVALID_ID; > > map->passive.status = PVCALLS_STATUS_BIND; > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return 0; > } > > @@ -695,21 +691,13 @@ int pvcalls_front_listen(struct socket *sock, int backlog) > struct xen_pvcalls_request *req; > int notify, req_id, ret; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > - return -ENOTCONN; > - } > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > + return PTR_ERR(map); > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (!map) { > - pvcalls_exit(); > - return -ENOTSOCK; > - } > - > if (map->passive.status != PVCALLS_STATUS_BIND) { > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EOPNOTSUPP; > } > > @@ -717,7 +705,7 @@ int pvcalls_front_listen(struct socket *sock, int backlog) > ret = get_request(bedata, &req_id); > if (ret < 0) { > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > req = RING_GET_REQUEST(&bedata->ring, req_id); > @@ -741,7 +729,7 @@ int pvcalls_front_listen(struct socket *sock, int backlog) > bedata->rsp[req_id].req_id = PVCALLS_INVALID_ID; > > map->passive.status = PVCALLS_STATUS_LISTEN; > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > > @@ -753,21 +741,13 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > struct xen_pvcalls_request *req; > int notify, req_id, ret, evtchn, nonblock; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > - return -ENOTCONN; > - } > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > + return PTR_ERR(map); > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (!map) { > - pvcalls_exit(); > - return -ENOTSOCK; > - } > - > if (map->passive.status != PVCALLS_STATUS_LISTEN) { > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EINVAL; > } > > @@ -785,13 +765,13 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > goto received; > } > if (nonblock) { > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EAGAIN; > } > if (wait_event_interruptible(map->passive.inflight_accept_req, > !test_and_set_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > (void *)&map->passive.flags))) { > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EINTR; > } > } > @@ -802,7 +782,7 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > (void *)&map->passive.flags); > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > map2 = kzalloc(sizeof(*map2), GFP_ATOMIC); > @@ -810,7 +790,7 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > (void *)&map->passive.flags); > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -ENOMEM; > } > ret = create_active(map2, &evtchn); > @@ -819,7 +799,7 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > (void *)&map->passive.flags); > spin_unlock(&bedata->socket_lock); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > list_add_tail(&map2->list, &bedata->socket_mappings); > @@ -841,13 +821,13 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > /* We could check if we have received a response before returning. */ > if (nonblock) { > WRITE_ONCE(map->passive.inflight_req_id, req_id); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EAGAIN; > } > > if (wait_event_interruptible(bedata->inflight_req, > READ_ONCE(bedata->rsp[req_id].req_id) == req_id)) { > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -EINTR; > } > /* read req_id, then the content */ > @@ -862,7 +842,7 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > (void *)&map->passive.flags); > pvcalls_front_free_map(bedata, map2); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return -ENOMEM; > } > newsock->sk->sk_send_head = (void *)map2; > @@ -874,7 +854,7 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) > clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, (void *)&map->passive.flags); > wake_up(&map->passive.inflight_accept_req); > > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > > @@ -965,23 +945,16 @@ unsigned int pvcalls_front_poll(struct file *file, struct socket *sock, > struct sock_mapping *map; > int ret; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map)) > return POLLNVAL; > - } > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (!map) { > - pvcalls_exit(); > - return POLLNVAL; > - } > if (map->active_socket) > ret = pvcalls_front_poll_active(file, bedata, map, wait); > else > ret = pvcalls_front_poll_passive(file, bedata, map, wait); > - pvcalls_exit(); > + pvcalls_exit_sock(sock); > return ret; > } > > @@ -995,25 +968,18 @@ int pvcalls_front_release(struct socket *sock) > if (sock->sk == NULL) > return 0; > > - pvcalls_enter(); > - if (!pvcalls_front_dev) { > - pvcalls_exit(); > + map = pvcalls_enter_sock(sock); > + if (IS_ERR(map) && PTR_ERR(map) == -ENOTCONN) > return -EIO; > - } > - > - bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > - > - map = (struct sock_mapping *) sock->sk->sk_send_head; > - if (map == NULL) { > - pvcalls_exit(); > + if (IS_ERR(map) && PTR_ERR(map) == -ENOTSOCK) > return 0; Hmm, what about: if (IS_ERR(map)) { if (PTR_ERR(map) == -ENOTCONN) return -EIO; else return 0; } Juergen