Message-ID: <CAO9qdTHnfMJFEOvENFGxestOWGE-5rzvA1XKdQn1y1KX-Sn=og@mail.gmail.com>
Date: Wed, 18 Jun 2025 01:04:15 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Richard Cochran <richardcochran@...il.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Yangbo Lu <yangbo.lu@....com>
Subject: Re: [PATCH net 1/2] ptp: fix breakage after ptp_vclock_in_use() rework

Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Mon, Jun 16, 2025 at 12:34:59AM +0900, Jeongjun Park wrote:
> > However, I don't think it is appropriate to fix ptp_vclock_in_use().
> > I agree that ptp->n_vclocks should be checked in the path where
> > ptp_clock_freerun() is called, but there are many drivers that do not
> > have any contact with ptp->n_vclocks in the path where
> > ptp_clock_unregister() is called.
>
> What do you mean there are many drivers that do not have any contact
> with ptp->n_vclocks? It is a feature visible only to the core, and
> transparent to the drivers. All drivers have contact with it, or none
> do. It all depends solely upon user configuration, and not dependent at
> all upon the specific driver.
>

I think I wrote it strangely, so I'll explain it again.

As you know, ptp_clock_{register,unregister}() is called not only in the
ptp core but also in several networking drivers to use the ptp clock
function. However, although these kernel drivers does not use any
n_vclocks-related functionality, the initial implementation of
ptp_vclock_in_use() acquired n_vclocks_mux and checked n_vclocks when
ptp_clock_unregister() was called.

> > The reason I removed the ptp->n_vclocks check logic from the
> > ptp_vclock_in_use() function is to prevent false positives from lockdep,
> > but also to prevent the performance overhead caused by locking
> > ptp->n_vclocks_mux and checking ptp->n_vclocks when calling
> > ptp_vclock_in_use() from a driver that has nothing to do with
> > ptp->n_vclocks.
>
> Can you quantify the performance overhead caused by acquiring
> ptp->n_vclocks_mux on unregistering physical clocks?
>

I think this performance overhead is a bit hard to quantify, but I think
it's wrong to add code inside ptp_clock_unregister() that performs
unnecessary locking in most cases except for some special cases.

> >
> > Therefore, I think it would be appropriate to modify ptp_clock_freerun()
> > like this instead of ptp_vclock_in_use():
> > ---
> >  drivers/ptp/ptp_private.h | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> > index 528d86a33f37..abd99087f0ca 100644
> > --- a/drivers/ptp/ptp_private.h
> > +++ b/drivers/ptp/ptp_private.h
> > @@ -104,10 +104,20 @@ static inline bool ptp_vclock_in_use(struct
> > ptp_clock *ptp)
> >  /* Check if ptp clock shall be free running */
> >  static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
> >  {
> > +   bool ret = false;
> > +
> >     if (ptp->has_cycles)
> > -       return false;
> > +       return ret;
> > +
> > +   if (mutex_lock_interruptible(&ptp->n_vclocks_mux))
> > +       return true;
> > +
> > +   if (ptp_vclock_in_use(ptp) && ptp->n_vclocks)
> > +       ret = true;
> > +
> > +   mutex_unlock(&ptp->n_vclocks_mux);
> >
> > -   return ptp_vclock_in_use(ptp);
> > +   return ret;
> >  }
> >
> >  extern const struct class ptp_class;
> > --
>
> If we leave the ptp_vclock_in_use() implementation as
> "return !ptp->is_virtual_clock;", then a physical PTP clock with
> n_vclocks=0 will have ptp_vclock_in_use() return true.
> Do you consider that expected behavior? What does "vclocks in use"
> even mean?
>
> In any case, I do agree with the fact that we shouldn't need to acquire
> a mutex in ptp_clock_unregister() to avoid racing with the sysfs device
> attributes. This seems avoidable with better operation ordering
> (unregister child virtual clocks when sysfs calls are no longer
> possible), or the use of pre-existing "ptp->defunct", or some other
> mechanism.
>
> You can certainly expand on that idea in net-next. The lockdep splat is
> a completely unrelated issue, and only that part is 'net' material.

I agree with this, so I think it would be a good idea to add the
ptp->n_vclocks check to the caller function that absolutely needs it,
like the patch I sent in the previous email, and to remove the
ptp_vclock_in_use() function altogether and replace it with the code that
checks ptp->is_virtual_clock.

Of course, I think this idea should be discussed through net-next after
this problems caused by the previous patch are resolved as you mentioned.

Regards,

Jeongjun Park

Powered by blists - more mailing lists