Re: [PATCH] net: remove unnecessary disc_data_lock in ppp line discipline

[Greg KH]

On Fri, May 21, 2021 at 03:57:26PM +0800, Gao Yan wrote:
> In tty layer, it use tty->ldisc_sem(using tty_ldisc_ref_wait and
> tty_ldisc_deref) to proect tty_ldisc_ops.
> 
> So I think tty->ldisc_sem can also protect tty->disc_data;
> For examlpe, When cpu A is running ppp_synctty_ioctl that hold
> the tty->ldisc_sem, at the same time  if cpu B calls ppp_synctty_close,
> it will wait until cpu A release tty->ldisc_sem.
> 
> So I think it is unnecessary to define additional disc_data_lock;
> 
> cpu A                           cpu B
> tty_ioctl                       tty_reopen
>  ->hold tty->ldisc_sem            ->hold tty->ldisc_sem(write), failed
>  ->ld->ops->ioctl                 ->wait...
>  ->release tty->ldisc_sem         ->wait...OK,hold tty->ldisc_sem
>                                     ->tty_ldisc_reinit
>                                       ->tty_ldisc_close
>                                         ->ld->ops->close
> 
> Signed-off-by: Gao Yan <gao.yanB@xxxxxxx>
> ---
>  drivers/net/ppp/ppp_async.c   | 11 ++---------
>  drivers/net/ppp/ppp_synctty.c | 11 ++---------
>  2 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_async.c b/drivers/net/ppp/ppp_async.c
> index 8b41aa3fb..7bc4846f5 100644
> --- a/drivers/net/ppp/ppp_async.c
> +++ b/drivers/net/ppp/ppp_async.c
> @@ -127,17 +127,13 @@ static const struct ppp_channel_ops async_ops = {
>   * FIXME: this is no longer true. The _close path for the ldisc is
>   * now guaranteed to be sane.
>   */
> -static DEFINE_RWLOCK(disc_data_lock);
>  
>  static struct asyncppp *ap_get(struct tty_struct *tty)
>  {
> -	struct asyncppp *ap;
> +	struct asyncppp *ap = tty->disc_data;
>  
> -	read_lock(&disc_data_lock);
> -	ap = tty->disc_data;
>  	if (ap != NULL)
>  		refcount_inc(&ap->refcnt);
> -	read_unlock(&disc_data_lock);
>  	return ap;
>  }
>  
> @@ -214,12 +210,9 @@ ppp_asynctty_open(struct tty_struct *tty)
>  static void
>  ppp_asynctty_close(struct tty_struct *tty)
>  {
> -	struct asyncppp *ap;
> +	struct asyncppp *ap = tty->disc_data;
>  
> -	write_lock_irq(&disc_data_lock);
> -	ap = tty->disc_data;
>  	tty->disc_data = NULL;
> -	write_unlock_irq(&disc_data_lock);
>  	if (!ap)
>  		return;
>  
> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
> index 576b6a93b..812f309c5 100644
> --- a/drivers/net/ppp/ppp_synctty.c
> +++ b/drivers/net/ppp/ppp_synctty.c
> @@ -129,17 +129,13 @@ ppp_print_buffer (const char *name, const __u8 *buf, int count)
>   *
>   * FIXME: Fixed in tty_io nowadays.
>   */
> -static DEFINE_RWLOCK(disc_data_lock);
>  
>  static struct syncppp *sp_get(struct tty_struct *tty)
>  {
> -	struct syncppp *ap;
> +	struct syncppp *ap = tty->disc_data;
>  
> -	read_lock(&disc_data_lock);
> -	ap = tty->disc_data;
>  	if (ap != NULL)
>  		refcount_inc(&ap->refcnt);
> -	read_unlock(&disc_data_lock);
>  	return ap;
>  }
>  
> @@ -213,12 +209,9 @@ ppp_sync_open(struct tty_struct *tty)
>  static void
>  ppp_sync_close(struct tty_struct *tty)
>  {
> -	struct syncppp *ap;
> +	struct syncppp *ap = tty->disc_data;
>  
> -	write_lock_irq(&disc_data_lock);
> -	ap = tty->disc_data;
>  	tty->disc_data = NULL;
> -	write_unlock_irq(&disc_data_lock);
>  	if (!ap)
>  		return;
>  
> -- 
> 2.17.1
> 

So removing this lock is ok?

How did you test this?  Is there anything wrong with keeping the
existing lock?  Does it show up in a real-world workload as being a
problem?  Unconstested locks should be almost a no-op.

thanks,

greg k-h