Messages in this thread | | | Date | Fri, 21 May 2021 10:33:05 +0200 | From | Greg KH <> | Subject | Re: [PATCH] net: remove unnecessary disc_data_lock in ppp line discipline |
| |
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
| |