netdev - Re: [PATCHv2 net-next 1/3] mlx4: introduce the time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAF2d9jjE4PDNWGjxrjVO8LuTaO0Azdz6s1HMTb=-8u+ABkBf1Q@mail.gmail.com>
Date: Mon, 28 Oct 2024 11:15:50 -0700
From: Mahesh Bandewar (महेश बंडेवार) <maheshb@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Netdev <netdev@...r.kernel.org>, Tariq Toukan <tariqt@...dia.com>, 
	Yishai Hadas <yishaih@...dia.com>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>, 
	Richard Cochran <richardcochran@...il.com>, Mahesh Bandewar <mahesh@...dewar.net>
Subject: Re: [PATCHv2 net-next 1/3] mlx4: introduce the time_cache into the
 mlx4 PTP implementation

On Thu, Oct 17, 2024 at 2:20 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 10/12/24 13:47, Mahesh Bandewar wrote:
> > The mlx4_clock_read() function, when invoked by cycle_counter->read(),
> > previously returned only the raw cycle count. However, for PTP helpers
> > like gettimex64(), which require both pre- and post-timestamps,
> > returning just the raw cycles is insufficient; the necessary
> > timestamps must also be provided.
> >
> > This patch introduces the time_cache into the implementation. As a
> > result, mlx4_en_read_clock() is now responsible for reading and
> > updating the clock_cache. This allows the function
> > mlx4_en_read_clock_cache() to serve as the cycle reader for
> > cycle_counter->read(), maintaining the same interface
> >
> > Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
> > ---
> >   drivers/net/ethernet/mellanox/mlx4/en_clock.c | 28 +++++++++++++++----
> >   drivers/net/ethernet/mellanox/mlx4/main.c     |  1 -
> >   drivers/net/ethernet/mellanox/mlx4/mlx4_en.h  |  1 +
> >   3 files changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > index cd754cd76bde..cab9221a0b26 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > @@ -36,15 +36,22 @@
> >
> >   #include "mlx4_en.h"
> >
> > -/* mlx4_en_read_clock - read raw cycle counter (to be used by time counter)
> > +/* mlx4_en_read_clock_cache - read cached raw cycle counter (to be
> > + * used by time counter)
> >    */
> > -static u64 mlx4_en_read_clock(const struct cyclecounter *tc)
> > +static u64 mlx4_en_read_clock_cache(const struct cyclecounter *tc)
> >   {
> >       struct mlx4_en_dev *mdev =
> >               container_of(tc, struct mlx4_en_dev, cycles);
> > -     struct mlx4_dev *dev = mdev->dev;
> >
> > -     return mlx4_read_clock(dev) & tc->mask;
> > +     return READ_ONCE(mdev->clock_cache) & tc->mask;
> > +}
> > +
> > +static void mlx4_en_read_clock(struct mlx4_en_dev *mdev)
> > +{
> > +     u64 cycles = mlx4_read_clock(mdev->dev);
> > +
> > +     WRITE_ONCE(mdev->clock_cache, cycles);
> >   }
> >
> >   u64 mlx4_en_get_cqe_ts(struct mlx4_cqe *cqe)
> > @@ -109,6 +116,9 @@ void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> >
> >       if (timeout) {
> >               write_seqlock_irqsave(&mdev->clock_lock, flags);
> > +             /* refresh the clock_cache */
> > +             mlx4_en_read_clock(mdev);
>
> It looks like you could make patch 2/3 much smaller introducing an
> explit cache_refresh() helper, that will not take the 2nd argument and
> using it where needed.
>
> Possibly even more importantly, why do you need a cache at all? I guess
> you could use directly mlx4_read_clock() in mlx4_en_phc_gettimex(), and
> implement mlx4_en_read_clock as:
>
> static void mlx4_en_read_clock(struct mlx4_en_dev *mdev)
> {
>         return mlx4_read_clock(mdev->dev, NULL);
> }
>
Pre//post timestamps collections must be performed at that time of
reading the PTP clock to be precise / accurate. The infusion of
time-cache is to ensure that the cyclecounter->read() uses the same
value that we read during the mlx4_read_clock().

One can do as you have suggested (i.e. none of the 1, & 2 will be needed)

e.g.
mlx4_en_phc_gettimex()
{
         write_seqlock_irqsave(&mdev->clock_lock, flags);
         ptp_read_system_prets();
         ns = timecounter_read(&mdev->clock);
         ptp_read_system_posts();
         write_sequnlock_irqrestore(&mdev->clock_lock, flags);
}

However, this will not measure precisely the width of the PTP-clock
read call since subsequent calculations in timecounter_read_delta()
and cyclecounter_cyc2ns() within  timecounter_read() will take place
before the ptp_read_system_postts() call.

> Does the cache give some extra gain I can't see? If so, it should be
> explained somewhere in the commit message.

As mentioned above, the time_cache makes the cyclecounter->read()
passive so that only one clock read op is performed. If you want I can
add the above explanation in the cover-letter?

> > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> > index febeadfdd5a5..9408313b0f4d 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> > @@ -1946,7 +1946,6 @@ u64 mlx4_read_clock(struct mlx4_dev *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(mlx4_read_clock);
> >
> > -
>
> Please don't introduce unrelated whitespace changes
>
Ack.

Thanks for the review,
--mahesh..

> Thanks,
>
> Paolo
>