| Message ID | 20241114114820.1411660-1-vadfed@meta.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c7a21af711e846a844095ae474f0f7e0ea8c6967 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] bnxt_en: optimize gettimex64 | expand |
On Thu, Nov 14, 2024 at 3:48 AM Vadim Fedorenko <vadfed@meta.com> wrote: > > Current implementation of gettimex64() makes at least 3 PCIe reads to > get current PHC time. It takes at least 2.2us to get this value back to > userspace. At the same time there is cached value of upper bits of PHC > available for packet timestamps already. This patch reuses cached value > to speed up reading of PHC time. > > Signed-off-by: Vadim Fedorenko <vadfed@meta.com> > --- > v1 -> v2: > * move cycles extension to a helper function and reuse it for both > timestamp extension and gettimex64() function Thanks. Reviewed-by: Michael Chan <michael.chan@broadcom.com>
On 11/14/2024 3:48 AM, Vadim Fedorenko wrote: > Current implementation of gettimex64() makes at least 3 PCIe reads to > get current PHC time. It takes at least 2.2us to get this value back to > userspace. At the same time there is cached value of upper bits of PHC > available for packet timestamps already. This patch reuses cached value > to speed up reading of PHC time. > > Signed-off-by: Vadim Fedorenko <vadfed@meta.com> > --- > v1 -> v2: > * move cycles extension to a helper function and reuse it for both > timestamp extension and gettimex64() function > > I did some benchmarks on host with Broadcom Thor NIC trying to build > histogram of time spent to call clock_gettime() to query PTP device > over million iterations. > With current implementation the result is (long tail is cut): > > 2200ns: 902624 > 2300ns: 87404 > 2400ns: 4025 > 2500ns: 1307 > 2600ns: 581 > 2700ns: 261 > 2800ns: 104 > 2900ns: 36 > 3000ns: 32 > 3100ns: 24 > 3200ns: 16 > 3300ns: 29 > 3400ns: 29 > 3500ns: 23 > > Optimized version on the very same machine and NIC gives next values: > > 900ns: 865436 > 1000ns: 128630 > 1100ns: 2671 > 1200ns: 727 > 1300ns: 397 > 1400ns: 178 > 1500ns: 92 > 1600ns: 16 > 1700ns: 15 > 1800ns: 11 > 1900ns: 6 > 2000ns: 20 > 2100ns: 11 > > That means pct(99) improved from 2300ns to 1000ns. > --- The driver already has to read and cache the values, so there's not much value in repeating that every CLOCK_GETTIME system call. This also simplifies the system timestamp process, and avoids the duplicate reads. Clever! Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 14 Nov 2024 03:48:20 -0800 you wrote: > Current implementation of gettimex64() makes at least 3 PCIe reads to > get current PHC time. It takes at least 2.2us to get this value back to > userspace. At the same time there is cached value of upper bits of PHC > available for packet timestamps already. This patch reuses cached value > to speed up reading of PHC time. > > Signed-off-by: Vadim Fedorenko <vadfed@meta.com> > > [...] Here is the summary with links: - [net-next,v2] bnxt_en: optimize gettimex64 https://git.kernel.org/netdev/net-next/c/c7a21af711e8 You are awesome, thank you!
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c index 91e7e08fabb1..075ccd589845 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c @@ -112,6 +112,28 @@ static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts, return rc; } +static int bnxt_refclk_read_low(struct bnxt *bp, struct ptp_system_timestamp *sts, + u32 *low) +{ + struct bnxt_ptp_cfg *ptp = bp->ptp_cfg; + unsigned long flags; + + /* We have to serialize reg access and FW reset */ + read_seqlock_excl_irqsave(&ptp->ptp_lock, flags); + + if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) { + read_sequnlock_excl_irqrestore(&ptp->ptp_lock, flags); + return -EIO; + } + + ptp_read_system_prets(sts); + *low = readl(bp->bar0 + ptp->refclk_mapped_regs[0]); + ptp_read_system_postts(sts); + + read_sequnlock_excl_irqrestore(&ptp->ptp_lock, flags); + return 0; +} + static void bnxt_ptp_get_current_time(struct bnxt *bp) { struct bnxt_ptp_cfg *ptp = bp->ptp_cfg; @@ -163,12 +185,14 @@ static int bnxt_ptp_gettimex(struct ptp_clock_info *ptp_info, struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg, ptp_info); u64 ns, cycles; + u32 low; int rc; - rc = bnxt_refclk_read(ptp->bp, sts, &cycles); + rc = bnxt_refclk_read_low(ptp->bp, sts, &low); if (rc) return rc; + cycles = bnxt_extend_cycles_32b_to_48b(ptp, low); ns = bnxt_timecounter_cyc2time(ptp, cycles); *ts = ns_to_timespec64(ns); @@ -801,15 +825,11 @@ void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod) int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts) { struct bnxt_ptp_cfg *ptp = bp->ptp_cfg; - u64 time; if (!ptp) return -ENODEV; - time = (u64)READ_ONCE(ptp->old_time) << BNXT_HI_TIMER_SHIFT; - *ts = (time & BNXT_HI_TIMER_MASK) | pkt_ts; - if (pkt_ts < (time & BNXT_LO_TIMER_MASK)) - *ts += BNXT_LO_TIMER_MASK + 1; + *ts = bnxt_extend_cycles_32b_to_48b(ptp, pkt_ts); return 0; } diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h index 4df4c2f373e0..c7851f8c971c 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h @@ -182,4 +182,15 @@ static inline u64 bnxt_timecounter_cyc2time(struct bnxt_ptp_cfg *ptp, u64 ts) return ns; } + +static inline u64 bnxt_extend_cycles_32b_to_48b(struct bnxt_ptp_cfg *ptp, u32 ts) +{ + u64 time, cycles; + + time = (u64)READ_ONCE(ptp->old_time) << BNXT_HI_TIMER_SHIFT; + cycles = (time & BNXT_HI_TIMER_MASK) | ts; + if (ts < (time & BNXT_LO_TIMER_MASK)) + cycles += BNXT_LO_TIMER_MASK + 1; + return cycles; +} #endif
Current implementation of gettimex64() makes at least 3 PCIe reads to get current PHC time. It takes at least 2.2us to get this value back to userspace. At the same time there is cached value of upper bits of PHC available for packet timestamps already. This patch reuses cached value to speed up reading of PHC time. Signed-off-by: Vadim Fedorenko <vadfed@meta.com> --- v1 -> v2: * move cycles extension to a helper function and reuse it for both timestamp extension and gettimex64() function I did some benchmarks on host with Broadcom Thor NIC trying to build histogram of time spent to call clock_gettime() to query PTP device over million iterations. With current implementation the result is (long tail is cut): 2200ns: 902624 2300ns: 87404 2400ns: 4025 2500ns: 1307 2600ns: 581 2700ns: 261 2800ns: 104 2900ns: 36 3000ns: 32 3100ns: 24 3200ns: 16 3300ns: 29 3400ns: 29 3500ns: 23 Optimized version on the very same machine and NIC gives next values: 900ns: 865436 1000ns: 128630 1100ns: 2671 1200ns: 727 1300ns: 397 1400ns: 178 1500ns: 92 1600ns: 16 1700ns: 15 1800ns: 11 1900ns: 6 2000ns: 20 2100ns: 11 That means pct(99) improved from 2300ns to 1000ns. --- drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 32 +++++++++++++++---- drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 11 +++++++ 2 files changed, 37 insertions(+), 6 deletions(-)