Re: [Intel-wired-lan] [PATCH next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, Feb 27, 2023 at 09:45:34PM -0800, Vinicius Costa Gomes wrote:
> ptp->aux_worker is a kthread and allows the user to set it's priority,
> which is useful for workloads that time synchronization is required.
> 
> As an optimization, when the interrupt is handled try to retrieve the
> timestamps "inline", if they are not, schedule the workload. This
> should reduce the delay before the TX timestamp is available to
> userspace.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  2 +-
>  drivers/net/ethernet/intel/igc/igc_main.c |  7 +++++-
>  drivers/net/ethernet/intel/igc/igc_ptp.c  | 29 ++++++++++++++++-------
>  3 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index f0617838a16a..ce7754cb7e6f 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -237,7 +237,6 @@ struct igc_adapter {
>  
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
> -	struct work_struct ptp_tx_work;
>  	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
>  	 * ptp_tx_lock.
>  	 */
> @@ -651,6 +650,7 @@ int igc_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
>  int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
>  void igc_ptp_tx_hang(struct igc_adapter *adapter);
>  void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
> +long igc_ptp_tx_work(struct ptp_clock_info *ptp);
>  
>  #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 4861ad0689ed..a7775d618867 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -5226,8 +5226,13 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
>  	}
>  
>  	if (tsicr & IGC_TSICR_TXTS) {
> +		long delay;
>  		/* retrieve hardware timestamp */
> -		schedule_work(&adapter->ptp_tx_work);
> +		delay = igc_ptp_tx_work(&adapter->ptp_caps);
> +		if (delay >= 0) {
> +			/* The timestamp is not ready, schedule to check later */
> +			ptp_schedule_worker(adapter->ptp_clock, delay);
> +		}
>  		ack |= IGC_TSICR_TXTS;
>  	}
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 0cb932b52a7b..34237464f26d 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -713,24 +713,37 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter, u32 mask)
>   * igc_ptp_tx_work
>   * @work: pointer to work struct
>   *
> - * This work function polls the TSYNCTXCTL valid bit to determine when a
> - * timestamp has been taken for the current stored skb.
> + * This work function polls the TSYNCTXCTL valid bit to determine when
> + * a timestamp has been taken for the current stored skb. Return a
> + * delay in case there's no timestamp ready.
>   */
> -static void igc_ptp_tx_work(struct work_struct *work)
> +long igc_ptp_tx_work(struct ptp_clock_info *ptp)
>  {
> -	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
> -						   ptp_tx_work);
> +	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
> +						   ptp_caps);
>  	struct igc_hw *hw = &adapter->hw;
>  	unsigned long flags;
>  	u32 tsynctxctl;
> +	long delay = -1;

idk what the coding style is in the igc driver, but this line is longer
than the previous one, and with network drivers, one usually prefers the
"reverse Christmas tree".

>  
>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  
>  	tsynctxctl = rd32(IGC_TSYNCTXCTL);
> +	tsynctxctl &= IGC_TSYNCTXCTL_TXTT_ANY;
> +	if (!tsynctxctl) {
> +		/* We got the interrupt but the timestamp is not ready
> +		 * still, schedule to check later.
> +		 */

this comment raises many hairs reading it, it sure makes it sound like the
hardware is buggy for raising an interrupt before the data is available.

I had this same question a while ago, could you borrow some of the wording
you used back then to explain in this comment, here, how it is possible to
get IGC_TSICR_TXTS ("Transmit Timestamp") set in the Time Sync Interrupt
Causes register, and for there not to be any TX timestamps available?
https://lore.kernel.org/netdev/20220815222639.346wachaaq5zjwue@skbuf/

> +		delay = usecs_to_jiffies(1);

it will surely take more than 1 us anyway to schedule, and so, setting a
timer for the kthread seems slightly pointless? The ptp_schedule_worker()/
__kthread_queue_delayed_work() API accepts 0 for delay, meaning "immediately".

> +		goto unlock;
> +	}
>  
> -	igc_ptp_tx_hwtstamp(adapter, tsynctxctl & IGC_TSYNCTXCTL_TXTT_ANY);
> +	igc_ptp_tx_hwtstamp(adapter, tsynctxctl);
>  
> +unlock:
>  	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
> +
> +	return delay;

On the same note: from the kthread you can also busy poll for a while;
for TX timestamps (which should be delivered rather quickly) I guess
this is more productive than rescheduling again (with such a short
interval), since it makes the slot available quicker for TX. Though,
I have to say, not quite clear how rescheduling ends up being needed in
the first place....

>  }
>  
>  /**
> @@ -993,6 +1006,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>  		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
>  		adapter->ptp_caps.n_pins = IGC_N_SDP;
>  		adapter->ptp_caps.verify = igc_ptp_verify_pin;
> +		adapter->ptp_caps.do_aux_work = igc_ptp_tx_work;
>  
>  		if (!igc_is_crosststamp_supported(adapter))
>  			break;
> @@ -1006,7 +1020,6 @@ void igc_ptp_init(struct igc_adapter *adapter)
>  
>  	spin_lock_init(&adapter->tmreg_lock);
>  	spin_lock_init(&adapter->ptp_tx_lock);
> -	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
>  
>  	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
>  	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
> @@ -1081,7 +1094,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
>  	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
>  		return;
>  
> -	cancel_work_sync(&adapter->ptp_tx_work);
> +	ptp_cancel_worker_sync(adapter->ptp_clock);
>  
>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  
> -- 
> 2.39.2
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan