Date:   Tue, 31 Jan 2023 11:39:33 +0900
From:   Sergey Senozhatsky <senozhatsky@chromium.org>
To:     Dawei Li <set_pte_at@outlook.com>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        linkinjeon@kernel.org, sfrench@samba.org, tom@talpey.com,
        hyc.lee@gmail.com, linux-cifs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct
 access on conn list
Message-ID: <Y9h/ZbLPQlbKiRzZ@google.com>
References: <20230115103209.146002-1-set_pte_at@outlook.com>
 <TYCP286MB23235FDD8102162698EF3154CAC09@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
 <Y9dEZ5IgfwpZNlVm@google.com>
 <TYCP286MB23230E29CC81F5C0590C59C9CAD39@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <TYCP286MB23230E29CC81F5C0590C59C9CAD39@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
Precedence: bulk

Hi,

On (23/01/30 22:16), Dawei Li wrote:
> Hi Sergey,
> 
> Thanks for reviewing,
> 
> On Mon, Jan 30, 2023 at 01:15:35PM +0900, Sergey Senozhatsky wrote:
> > On (23/01/15 18:32), Dawei Li wrote:
> > > 
> > >  void ksmbd_conn_free(struct ksmbd_conn *conn)
> > >  {
> > > -	write_lock(&conn_list_lock);
> > > -	list_del(&conn->conns_list);
> > > -	write_unlock(&conn_list_lock);
> > > +	spin_lock(&conn_list_lock);
> > > +	list_del_rcu(&conn->conns_list);
> > > +	spin_unlock(&conn_list_lock);
>         synchronize_rcu(); 
> > >  
> > >  	xa_destroy(&conn->sessions);
> > >  	kvfree(conn->request_buf);
> > 
> > From a quick look this does not seem like a correct RCU usage. E.g.
> > where do you wait for grace periods and synchronize readers/writers?
> 
> Nice catch, I totally mess it up. Thanks!
> 
> At first glance, I assume synchronize_rcu() will do the job if sleeping
> is OK?

Yes, synchronize_rcu() will sleep (schedule()) and wait for grace
period to expire (and synchronize will all the RCU readers). RCU
is good for cases when writes are seldom, which may not be the case
with ksmb.

I really want to see benhcmarks, why do we want to remove the RW-lock.