All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: remove unnecessary disc_data_lock in ppp line discipline
@ 2021-05-21  7:57 Gao Yan
  2021-05-21  8:33 ` Greg KH
  0 siblings, 1 reply; 2+ messages in thread
From: Gao Yan @ 2021-05-21  7:57 UTC (permalink / raw)
  To: gregkh, jirislaby, paulus, davem, kuba, linux-kernel; +Cc: Gao Yan

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@h3c.com>
---
 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


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] net: remove unnecessary disc_data_lock in ppp line discipline
  2021-05-21  7:57 [PATCH] net: remove unnecessary disc_data_lock in ppp line discipline Gao Yan
@ 2021-05-21  8:33 ` Greg KH
  0 siblings, 0 replies; 2+ messages in thread
From: Greg KH @ 2021-05-21  8:33 UTC (permalink / raw)
  To: Gao Yan; +Cc: jirislaby, paulus, davem, kuba, linux-kernel

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@h3c.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-05-21  9:22 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-21  7:57 [PATCH] net: remove unnecessary disc_data_lock in ppp line discipline Gao Yan
2021-05-21  8:33 ` Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.