[Intel-wired-lan] [PATCH next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps
Vinicius Costa Gomes
vinicius.gomes at intel.com
Thu Mar 9 21:58:49 UTC 2023
Vladimir Oltean <vladimir.oltean at nxp.com> writes:
> 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".
>
Ugh. My fault. Will fix.
>>
>> 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.
>
Another good point. I will check how many times this condition happens
and see if the hardware folks have any suggestion about what could be
done. I agree that having this re-schedule indeed looks like buggy
hardware.
> 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/
>
Will improve the comments, if this check still needs to stay.
>> + 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".
>
The '1' is more to mean "do something else" and then check again. From
looking at code, '0' could mean that it could run just before this
kthread returns to the scheduler. But let's see if the hardware folks
have better suggestions (and if this is indeed necessary).
>> + 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....
>
Again, let's see what my experiments and the HW folks say about this.
Perhaps this is just me being paranoid.
>> }
>>
>> /**
>> @@ -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
>>
Cheers,
--
Vinicius
More information about the Intel-wired-lan
mailing list