[PATCHv2 net-next 1/3] mlx4: introduce the time_cache into the mlx4 PTP implementation

[PATCHv2 net-next 1/3] mlx4: introduce the time_cache into the mlx4 PTP implementation

[Mahesh Bandewar]

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@google.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);
+
 		timecounter_read(&mdev->clock);
 		write_sequnlock_irqrestore(&mdev->clock_lock, flags);
 		mdev->last_overflow_check = jiffies;
@@ -135,6 +145,8 @@ static int mlx4_en_phc_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	mult = (u32)adjust_by_scaled_ppm(mdev->nominal_c_mult, scaled_ppm);
 
 	write_seqlock_irqsave(&mdev->clock_lock, flags);
+	/* refresh the clock_cache */
+	mlx4_en_read_clock(mdev);
 	timecounter_read(&mdev->clock);
 	mdev->cycles.mult = mult;
 	write_sequnlock_irqrestore(&mdev->clock_lock, flags);
@@ -179,6 +191,8 @@ static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp,
 	u64 ns;
 
 	write_seqlock_irqsave(&mdev->clock_lock, flags);
+	/* refresh the clock_cache */
+	mlx4_en_read_clock(mdev);
 	ns = timecounter_read(&mdev->clock);
 	write_sequnlock_irqrestore(&mdev->clock_lock, flags);
 
@@ -205,6 +219,8 @@ static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
 
 	/* reset the timecounter */
 	write_seqlock_irqsave(&mdev->clock_lock, flags);
+	/* refresh the clock_cache */
+	mlx4_en_read_clock(mdev);
 	timecounter_init(&mdev->clock, &mdev->cycles, ns);
 	write_sequnlock_irqrestore(&mdev->clock_lock, flags);
 
@@ -273,7 +289,7 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	seqlock_init(&mdev->clock_lock);
 
 	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
-	mdev->cycles.read = mlx4_en_read_clock;
+	mdev->cycles.read = mlx4_en_read_clock_cache;
 	mdev->cycles.mask = CLOCKSOURCE_MASK(48);
 	mdev->cycles.shift = freq_to_shift(dev->caps.hca_core_clock);
 	mdev->cycles.mult =
@@ -281,6 +297,8 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	mdev->nominal_c_mult = mdev->cycles.mult;
 
 	write_seqlock_irqsave(&mdev->clock_lock, flags);
+	/* initialize the clock_cache */
+	mlx4_en_read_clock(mdev);
 	timecounter_init(&mdev->clock, &mdev->cycles,
 			 ktime_to_ns(ktime_get_real()));
 	write_sequnlock_irqrestore(&mdev->clock_lock, flags);
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);
 
-
 static int map_internal_clock(struct mlx4_dev *dev)
 {
 	struct mlx4_priv *priv = mlx4_priv(dev);
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index 28b70dcc652e..077b529eb01a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -435,6 +435,7 @@ struct mlx4_en_dev {
 	unsigned long		last_overflow_check;
 	struct ptp_clock	*ptp_clock;
 	struct ptp_clock_info	ptp_clock_info;
+	u64			clock_cache;
 	struct notifier_block	netdev_nb;
 	struct notifier_block	mlx_nb;
 };
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCHv2 net-next 1/3] mlx4: introduce the time_cache into the mlx4 PTP implementation

[Paolo Abeni]

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@google.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);
}

Does the cache give some extra gain I can't see? If so, it should be 
explained somewhere in the commit message.
> 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

Thanks,

Paolo

Re: [PATCHv2 net-next 1/3] mlx4: introduce the time_cache into the mlx4 PTP implementation

[Mahesh Bandewar (महेश बंडेवार)]

On Thu, Oct 17, 2024 at 2:20 AM Paolo Abeni <pabeni@redhat.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@google.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
>