[Intel-wired-lan] [PATCH next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps
Vladimir Oltean
vladimir.oltean at nxp.com
Tue Feb 28 18:16:26 UTC 2023
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 at 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
>
More information about the Intel-wired-lan
mailing list