From mboxrd@z Thu Jan 1 00:00:00 1970 From: Santosh Shilimkar Subject: Re: [PATCH V2 net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock Date: Wed, 14 Feb 2018 10:38:33 -0800 Message-ID: <7da00869-2fb4-c4d2-f8da-26313c83178c@oracle.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, rds-devel@oss.oracle.com To: Sowmini Varadhan , netdev@vger.kernel.org, willemdebruijn.kernel@gmail.com Return-path: Received: from userp2130.oracle.com ([156.151.31.86]:34768 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1162010AbeBNSij (ORCPT ); Wed, 14 Feb 2018 13:38:39 -0500 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2/14/2018 2:28 AM, Sowmini Varadhan wrote: > The existing model holds a reference from the rds_sock to the > rds_message, but the rds_message does not itself hold a sock_put() > on the rds_sock. Instead the m_rs field in the rds_message is > assigned when the message is queued on the sock, and nulled when > the message is dequeued from the sock. > > We want to be able to notify userspace when the rds_message > is actually freed (from rds_message_purge(), after the refcounts > to the rds_message go to 0). At the time that rds_message_purge() > is called, the message is no longer on the rds_sock retransmit > queue. Thus the explicit reference for the m_rs is needed to > send a notification that will signal to userspace that > it is now safe to free/reuse any pages that may have > been pinned down for zerocopy. > > This patch manages the m_rs assignment in the rds_message with > the necessary refcount book-keeping. > > Signed-off-by: Sowmini Varadhan > --- [...] > @@ -756,9 +755,6 @@ void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in *dest) > */ > if (!test_and_clear_bit(RDS_MSG_ON_CONN, &rm->m_flags)) { > spin_unlock_irqrestore(&cp->cp_lock, flags); > - spin_lock_irqsave(&rm->m_rs_lock, flags); > - rm->m_rs = NULL; > - spin_unlock_irqrestore(&rm->m_rs_lock, flags); > continue; > } > list_del_init(&rm->m_conn_item); This hunk was clearly wrong so good that you got rid of it as well. Patch looks fine to me. Acked-by: Santosh Shilimkar