Re: [PATCH net-next] net/mlx5: use do_aux_work for PHC overflow checks

[Tariq Toukan <ttoukan.linux@gmail.com>]

On 04/01/2025 0:25, Vadim Fedorenko wrote:
> On 22/12/2024 14:10, Dragos Tatulea wrote:
>> On Tue Dec 17, 2024 at 8:57 PM CET, Vadim Fedorenko wrote:
>>> The overflow_work is using system wq to do overflow checks and updates
>>> for PHC device timecounter, which might be overhelmed by other tasks.
>>> But there is dedicated kthread in PTP subsystem designed for such
>>> things. This patch changes the work queue to proper align with PTP
>>> subsystem and to avoid overloading system work queue.
>>> The adjfine() function acts the same way as overflow check worker,
>>> we can postpone ptp aux worker till the next overflow period after
>>> adjfine() was called.
>>>
>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>> ---
>>>   .../ethernet/mellanox/mlx5/core/lib/clock.c   | 25 +++++++++++--------
>>>   include/linux/mlx5/driver.h                   |  1 -
>>>   2 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/ 
>>> drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
>>> index 4822d01123b4..ff3780331273 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
>>> @@ -322,17 +322,16 @@ static void mlx5_pps_out(struct work_struct *work)
>>>       }
>>>   }
>>> -static void mlx5_timestamp_overflow(struct work_struct *work)
>>> +static long mlx5_timestamp_overflow(struct ptp_clock_info *ptp_info)
>>>   {
>>> -    struct delayed_work *dwork = to_delayed_work(work);
>>>       struct mlx5_core_dev *mdev;
>>>       struct mlx5_timer *timer;
>>>       struct mlx5_clock *clock;
>>>       unsigned long flags;
>>> -    timer = container_of(dwork, struct mlx5_timer, overflow_work);
>>> -    clock = container_of(timer, struct mlx5_clock, timer);
>>> +    clock = container_of(ptp_info, struct mlx5_clock, ptp_info);
>>>       mdev = container_of(clock, struct mlx5_core_dev, clock);
>>> +    timer = &clock->timer;
>>>       if (mdev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR)
>>>           goto out;
>>> @@ -343,7 +342,7 @@ static void mlx5_timestamp_overflow(struct 
>>> work_struct *work)
>>>       write_sequnlock_irqrestore(&clock->lock, flags);
>>>   out:
>>> -    schedule_delayed_work(&timer->overflow_work, timer- 
>>> >overflow_period);
>>> +    return timer->overflow_period;
>>>   }
>>>   static int mlx5_ptp_settime_real_time(struct mlx5_core_dev *mdev,
>>> @@ -517,6 +516,7 @@ static int mlx5_ptp_adjfine(struct ptp_clock_info 
>>> *ptp, long scaled_ppm)
>>>       timer->cycles.mult = mult;
>>>       mlx5_update_clock_info_page(mdev);
>>>       write_sequnlock_irqrestore(&clock->lock, flags);
>>> +    ptp_schedule_worker(clock->ptp, timer->overflow_period);
>>>       return 0;
>>>   }
>>> @@ -852,6 +852,7 @@ static const struct ptp_clock_info 
>>> mlx5_ptp_clock_info = {
>>>       .settime64    = mlx5_ptp_settime,
>>>       .enable        = NULL,
>>>       .verify        = NULL,
>>> +    .do_aux_work    = mlx5_timestamp_overflow,
>>>   };
>>>   static int mlx5_query_mtpps_pin_mode(struct mlx5_core_dev *mdev, u8 
>>> pin,
>>> @@ -1052,12 +1053,12 @@ static void mlx5_init_overflow_period(struct 
>>> mlx5_clock *clock)
>>>       do_div(ns, NSEC_PER_SEC / HZ);
>>>       timer->overflow_period = ns;
>>> -    INIT_DELAYED_WORK(&timer->overflow_work, mlx5_timestamp_overflow);
>>> -    if (timer->overflow_period)
>>> -        schedule_delayed_work(&timer->overflow_work, 0);
>>> -    else
>>> +    if (!timer->overflow_period) {
>>> +        timer->overflow_period = HZ;
>>>           mlx5_core_warn(mdev,
>>> -                   "invalid overflow period, overflow_work is not 
>>> scheduled\n");
>>> +                   "invalid overflow period,"
>>> +                   "overflow_work is scheduled once per second\n");
>>> +    }
>>>       if (clock_info)
>>>           clock_info->overflow_period = timer->overflow_period;
>>> @@ -1172,6 +1173,9 @@ void mlx5_init_clock(struct mlx5_core_dev *mdev)
>>>       MLX5_NB_INIT(&clock->pps_nb, mlx5_pps_event, PPS_EVENT);
>>>       mlx5_eq_notifier_register(mdev, &clock->pps_nb);
>>> +
>>> +    if (clock->ptp)
>>> +        ptp_schedule_worker(clock->ptp, 0);
>>>   }
>>>   void mlx5_cleanup_clock(struct mlx5_core_dev *mdev)
>>> @@ -1188,7 +1192,6 @@ void mlx5_cleanup_clock(struct mlx5_core_dev 
>>> *mdev)
>>>       }
>>>       cancel_work_sync(&clock->pps_info.out_work);
>>> -    cancel_delayed_work_sync(&clock->timer.overflow_work);
>>>       if (mdev->clock_info) {
>>>           free_page((unsigned long)mdev->clock_info);
>>> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
>>> index fc7e6153b73d..3ac2fc1b52cf 100644
>>> --- a/include/linux/mlx5/driver.h
>>> +++ b/include/linux/mlx5/driver.h
>>> @@ -690,7 +690,6 @@ struct mlx5_timer {
>>>       struct timecounter         tc;
>>>       u32                        nominal_c_mult;
>>>       unsigned long              overflow_period;
>>> -    struct delayed_work        overflow_work;
>>>   };
>>>   struct mlx5_clock {
>>
>> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
>>
> 
> Hi Saeed, Tariq, Leon!
> 
> We need explicit Ack-by from official mlx5 maintainer here to let this
> patch go directly to net-next. Could you please check it?
> 
> Thanks,
> Vadim
> 
> 

Hi,

Back after a long vacation.
I'll reply shortly.

Regards,
Tariq