* [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter
@ 2024-10-29 20:54 Vadim Fedorenko
2024-10-29 20:54 ` [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock Vadim Fedorenko
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2024-10-29 20:54 UTC (permalink / raw)
To: Vadim Fedorenko, Michael Chan, Pavan Chebbi, Jakub Kicinski
Cc: Andrew Lunn, Paolo Abeni, David S. Miller, netdev,
Richard Cochran, Vadim Fedorenko
This hardware can provide only 48 bits of cycle counter. We can leave
only 24 bits in the cache to extend RX timestamps from 32 bits to 48
bits. Lower 8 bits of the cached value will be used to check for
roll-over while extending to full 48 bits.
This change makes cache writes atomic even on 32 bit platforms and we
can simply use READ_ONCE()/WRITE_ONCE() pair and remove spinlock. The
configuration structure will be also reduced by 4 bytes.
Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 8 ++++----
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 18 +++---------------
2 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index fa514be87650..820c7e83e586 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -106,7 +106,7 @@ static void bnxt_ptp_get_current_time(struct bnxt *bp)
if (!ptp)
return;
spin_lock_irqsave(&ptp->ptp_lock, flags);
- WRITE_ONCE(ptp->old_time, ptp->current_time);
+ WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
bnxt_refclk_read(bp, NULL, &ptp->current_time);
spin_unlock_irqrestore(&ptp->ptp_lock, flags);
}
@@ -174,7 +174,7 @@ void bnxt_ptp_update_current_time(struct bnxt *bp)
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
bnxt_refclk_read(ptp->bp, NULL, &ptp->current_time);
- WRITE_ONCE(ptp->old_time, ptp->current_time);
+ WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
}
static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
@@ -813,7 +813,7 @@ int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
if (!ptp)
return -ENODEV;
- BNXT_READ_TIME64(ptp, time, ptp->old_time);
+ 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;
@@ -1079,7 +1079,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
spin_lock_irqsave(&ptp->ptp_lock, flags);
bnxt_refclk_read(bp, NULL, &ptp->current_time);
- WRITE_ONCE(ptp->old_time, ptp->current_time);
+ WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
spin_unlock_irqrestore(&ptp->ptp_lock, flags);
ptp_schedule_worker(ptp->ptp_clock, 0);
}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index f322466ecad3..3ac5cbc1c5c4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -21,6 +21,7 @@
#define BNXT_DEVCLK_FREQ 1000000
#define BNXT_LO_TIMER_MASK 0x0000ffffffffUL
#define BNXT_HI_TIMER_MASK 0xffff00000000UL
+#define BNXT_HI_TIMER_SHIFT 24
#define BNXT_PTP_DFLT_TX_TMO 1000 /* ms */
#define BNXT_PTP_QTS_TIMEOUT 1000
@@ -106,10 +107,11 @@ struct bnxt_ptp_cfg {
/* serialize ts tx request queuing */
spinlock_t ptp_tx_lock;
u64 current_time;
- u64 old_time;
unsigned long next_period;
unsigned long next_overflow_check;
u32 cmult;
+ /* cache of upper 24 bits of cyclecoutner. 8 bits are used to check for roll-over */
+ u32 old_time;
/* a 23b shift cyclecounter will overflow in ~36 mins. Check overflow every 18 mins. */
#define BNXT_PHC_OVERFLOW_PERIOD (18 * 60 * HZ)
@@ -145,20 +147,6 @@ struct bnxt_ptp_cfg {
struct bnxt_ptp_stats stats;
};
-#if BITS_PER_LONG == 32
-#define BNXT_READ_TIME64(ptp, dst, src) \
-do { \
- unsigned long flags; \
- \
- spin_lock_irqsave(&(ptp)->ptp_lock, flags); \
- (dst) = (src); \
- spin_unlock_irqrestore(&(ptp)->ptp_lock, flags); \
-} while (0)
-#else
-#define BNXT_READ_TIME64(ptp, dst, src) \
- ((dst) = READ_ONCE(src))
-#endif
-
#define BNXT_PTP_INC_TX_AVAIL(ptp) \
do { \
spin_lock_bh(&(ptp)->ptp_tx_lock); \
--
2.43.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock
2024-10-29 20:54 [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Vadim Fedorenko
@ 2024-10-29 20:54 ` Vadim Fedorenko
2024-10-30 16:54 ` Michael Chan
2024-10-30 16:53 ` [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Michael Chan
2024-11-03 19:22 ` Jakub Kicinski
2 siblings, 1 reply; 6+ messages in thread
From: Vadim Fedorenko @ 2024-10-29 20:54 UTC (permalink / raw)
To: Vadim Fedorenko, Michael Chan, Pavan Chebbi, Jakub Kicinski
Cc: Andrew Lunn, Paolo Abeni, David S. Miller, netdev,
Richard Cochran, Vadim Fedorenko
We can see high contention on ptp_lock while doing RX timestamping
on high packet rates over several queues. Spinlock is not effecient
to protect timecounter for RX timestamps when reads are the most
usual operations and writes are only occasional. It's better to use
seqlock in such cases.
Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
v4:
- add sequnlock on error path in bnxt_refclk_read()
v3:
- remove unused variable
v2:
- use read_excl lock to serialize reg access with FW reset
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 19 +++--
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 77 +++++++------------
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 14 +++-
3 files changed, 49 insertions(+), 61 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6dd6541d8619..a7eabef6e7ea 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -2254,11 +2254,8 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
if (!bnxt_get_rx_ts_p5(bp, &ts, cmpl_ts)) {
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
- unsigned long flags;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
- ns = timecounter_cyc2time(&ptp->tc, ts);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ ns = bnxt_timecounter_cyc2time(ptp, ts);
memset(skb_hwtstamps(skb), 0,
sizeof(*skb_hwtstamps(skb)));
skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(ns);
@@ -2764,12 +2761,12 @@ static int bnxt_async_event_process(struct bnxt *bp,
if (!ptp)
goto async_event_process_exit;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
bnxt_ptp_update_current_time(bp);
ns = (((u64)BNXT_EVENT_PHC_RTC_UPDATE(data1) <<
BNXT_PHC_BITS) | ptp->current_time);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
bnxt_ptp_rtc_timecounter_init(ptp, ns);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
}
break;
}
@@ -13496,12 +13493,13 @@ static void bnxt_force_fw_reset(struct bnxt *bp)
test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
return;
+ /* we have to serialize with bnxt_refclk_read()*/
if (ptp) {
unsigned long flags;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
} else {
set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
}
@@ -13565,12 +13563,13 @@ void bnxt_fw_reset(struct bnxt *bp)
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
int n = 0, tmo;
+ /* we have to serialize with bnxt_refclk_read()*/
if (ptp) {
unsigned long flags;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
} else {
set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index 820c7e83e586..fa9adc4007d8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -67,21 +67,25 @@ static int bnxt_ptp_settime(struct ptp_clock_info *ptp_info,
if (BNXT_PTP_USE_RTC(ptp->bp))
return bnxt_ptp_cfg_settime(ptp->bp, ns);
- spin_lock_irqsave(&ptp->ptp_lock, flags);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
timecounter_init(&ptp->tc, &ptp->cc, ns);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
return 0;
}
-/* Caller holds ptp_lock */
static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
u64 *ns)
{
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
u32 high_before, high_now, low;
+ unsigned long flags;
- if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
+ /* 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;
+ }
high_before = readl(bp->bar0 + ptp->refclk_mapped_regs[1]);
ptp_read_system_prets(sts);
@@ -93,6 +97,7 @@ static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
low = readl(bp->bar0 + ptp->refclk_mapped_regs[0]);
ptp_read_system_postts(sts);
}
+ read_sequnlock_excl_irqrestore(&ptp->ptp_lock, flags);
*ns = ((u64)high_now << 32) | low;
return 0;
@@ -101,14 +106,11 @@ static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
static void bnxt_ptp_get_current_time(struct bnxt *bp)
{
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
- unsigned long flags;
if (!ptp)
return;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
bnxt_refclk_read(bp, NULL, &ptp->current_time);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
}
static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts,
@@ -151,24 +153,19 @@ 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);
- unsigned long flags;
u64 ns, cycles;
int rc;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
rc = bnxt_refclk_read(ptp->bp, sts, &cycles);
- if (rc) {
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ if (rc)
return rc;
- }
- ns = timecounter_cyc2time(&ptp->tc, cycles);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+
+ ns = bnxt_timecounter_cyc2time(ptp, cycles);
*ts = ns_to_timespec64(ns);
return 0;
}
-/* Caller holds ptp_lock */
void bnxt_ptp_update_current_time(struct bnxt *bp)
{
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
@@ -180,7 +177,6 @@ void bnxt_ptp_update_current_time(struct bnxt *bp)
static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
{
struct hwrm_port_mac_cfg_input *req;
- unsigned long flags;
int rc;
rc = hwrm_req_init(ptp->bp, req, HWRM_PORT_MAC_CFG);
@@ -194,9 +190,7 @@ static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
if (rc) {
netdev_err(ptp->bp->dev, "ptp adjphc failed. rc = %x\n", rc);
} else {
- spin_lock_irqsave(&ptp->ptp_lock, flags);
bnxt_ptp_update_current_time(ptp->bp);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
}
return rc;
@@ -211,9 +205,9 @@ static int bnxt_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
if (BNXT_PTP_USE_RTC(ptp->bp))
return bnxt_ptp_adjphc(ptp, delta);
- spin_lock_irqsave(&ptp->ptp_lock, flags);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
timecounter_adjtime(&ptp->tc, delta);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
return 0;
}
@@ -246,10 +240,10 @@ static int bnxt_ptp_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
if (!BNXT_MH(bp))
return bnxt_ptp_adjfine_rtc(bp, scaled_ppm);
- spin_lock_irqsave(&ptp->ptp_lock, flags);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
timecounter_read(&ptp->tc);
ptp->cc.mult = adjust_by_scaled_ppm(ptp->cmult, scaled_ppm);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
return 0;
}
@@ -257,13 +251,10 @@ void bnxt_ptp_pps_event(struct bnxt *bp, u32 data1, u32 data2)
{
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
struct ptp_clock_event event;
- unsigned long flags;
u64 ns, pps_ts;
pps_ts = EVENT_PPS_TS(data2, data1);
- spin_lock_irqsave(&ptp->ptp_lock, flags);
- ns = timecounter_cyc2time(&ptp->tc, pps_ts);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ ns = bnxt_timecounter_cyc2time(ptp, pps_ts);
switch (EVENT_DATA2_PPS_EVENT_TYPE(data2)) {
case ASYNC_EVENT_CMPL_PPS_TIMESTAMP_EVENT_DATA2_EVENT_TYPE_INTERNAL:
@@ -400,17 +391,13 @@ static int bnxt_get_target_cycles(struct bnxt_ptp_cfg *ptp, u64 target_ns,
{
u64 cycles_now;
u64 nsec_now, nsec_delta;
- unsigned long flags;
int rc;
- spin_lock_irqsave(&ptp->ptp_lock, flags);
rc = bnxt_refclk_read(ptp->bp, NULL, &cycles_now);
- if (rc) {
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ if (rc)
return rc;
- }
- nsec_now = timecounter_cyc2time(&ptp->tc, cycles_now);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+
+ nsec_now = bnxt_timecounter_cyc2time(ptp, cycles_now);
nsec_delta = target_ns - nsec_now;
*cycles_delta = div64_u64(nsec_delta << ptp->cc.shift, ptp->cc.mult);
@@ -697,7 +684,6 @@ static int bnxt_stamp_tx_skb(struct bnxt *bp, int slot)
struct skb_shared_hwtstamps timestamp;
struct bnxt_ptp_tx_req *txts_req;
unsigned long now = jiffies;
- unsigned long flags;
u64 ts = 0, ns = 0;
u32 tmo = 0;
int rc;
@@ -711,9 +697,7 @@ static int bnxt_stamp_tx_skb(struct bnxt *bp, int slot)
tmo, slot);
if (!rc) {
memset(×tamp, 0, sizeof(timestamp));
- spin_lock_irqsave(&ptp->ptp_lock, flags);
- ns = timecounter_cyc2time(&ptp->tc, ts);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ ns = bnxt_timecounter_cyc2time(ptp, ts);
timestamp.hwtstamp = ns_to_ktime(ns);
skb_tstamp_tx(txts_req->tx_skb, ×tamp);
ptp->stats.ts_pkts++;
@@ -767,9 +751,9 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
bnxt_ptp_get_current_time(bp);
ptp->next_period = now + HZ;
if (time_after_eq(now, ptp->next_overflow_check)) {
- spin_lock_irqsave(&ptp->ptp_lock, flags);
+ write_seqlock_irqsave(&ptp->ptp_lock, flags);
timecounter_read(&ptp->tc);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
ptp->next_overflow_check = now + BNXT_PHC_OVERFLOW_PERIOD;
}
if (rc == -EAGAIN)
@@ -829,7 +813,6 @@ void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
u32 opaque = tscmp->tx_ts_cmp_opaque;
struct bnxt_tx_ring_info *txr;
struct bnxt_sw_tx_bd *tx_buf;
- unsigned long flags;
u64 ts, ns;
u16 cons;
@@ -844,9 +827,7 @@ void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
le32_to_cpu(tscmp->tx_ts_cmp_flags_type),
le32_to_cpu(tscmp->tx_ts_cmp_errors_v));
} else {
- spin_lock_irqsave(&ptp->ptp_lock, flags);
- ns = timecounter_cyc2time(&ptp->tc, ts);
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
+ ns = bnxt_timecounter_cyc2time(ptp, ts);
timestamp.hwtstamp = ns_to_ktime(ns);
skb_tstamp_tx(tx_buf->skb, ×tamp);
}
@@ -1005,9 +986,9 @@ int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg)
if (rc)
return rc;
}
- spin_lock_irqsave(&bp->ptp_cfg->ptp_lock, flags);
+ write_seqlock_irqsave(&bp->ptp_cfg->ptp_lock, flags);
bnxt_ptp_rtc_timecounter_init(bp->ptp_cfg, ns);
- spin_unlock_irqrestore(&bp->ptp_cfg->ptp_lock, flags);
+ write_sequnlock_irqrestore(&bp->ptp_cfg->ptp_lock, flags);
return 0;
}
@@ -1042,7 +1023,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
bnxt_ptp_free(bp);
WRITE_ONCE(ptp->tx_avail, BNXT_MAX_TX_TS);
- spin_lock_init(&ptp->ptp_lock);
+ seqlock_init(&ptp->ptp_lock);
spin_lock_init(&ptp->ptp_tx_lock);
if (BNXT_PTP_USE_RTC(bp)) {
@@ -1075,12 +1056,8 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
atomic64_set(&ptp->stats.ts_err, 0);
if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
- unsigned long flags;
-
- spin_lock_irqsave(&ptp->ptp_lock, flags);
bnxt_refclk_read(bp, NULL, &ptp->current_time);
WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
- spin_unlock_irqrestore(&ptp->ptp_lock, flags);
ptp_schedule_worker(ptp->ptp_clock, 0);
}
ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index 3ac5cbc1c5c4..4df4c2f373e0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -103,7 +103,7 @@ struct bnxt_ptp_cfg {
struct timecounter tc;
struct bnxt_pps pps_info;
/* serialize timecounter access */
- spinlock_t ptp_lock;
+ seqlock_t ptp_lock;
/* serialize ts tx request queuing */
spinlock_t ptp_tx_lock;
u64 current_time;
@@ -170,4 +170,16 @@ void bnxt_ptp_rtc_timecounter_init(struct bnxt_ptp_cfg *ptp, u64 ns);
int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg);
int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg);
void bnxt_ptp_clear(struct bnxt *bp);
+static inline u64 bnxt_timecounter_cyc2time(struct bnxt_ptp_cfg *ptp, u64 ts)
+{
+ unsigned int seq;
+ u64 ns;
+
+ do {
+ seq = read_seqbegin(&ptp->ptp_lock);
+ ns = timecounter_cyc2time(&ptp->tc, ts);
+ } while (read_seqretry(&ptp->ptp_lock, seq));
+
+ return ns;
+}
#endif
--
2.43.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter
2024-10-29 20:54 [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Vadim Fedorenko
2024-10-29 20:54 ` [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock Vadim Fedorenko
@ 2024-10-30 16:53 ` Michael Chan
2024-11-03 19:22 ` Jakub Kicinski
2 siblings, 0 replies; 6+ messages in thread
From: Michael Chan @ 2024-10-30 16:53 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Vadim Fedorenko, Pavan Chebbi, Jakub Kicinski, Andrew Lunn,
Paolo Abeni, David S. Miller, netdev, Richard Cochran
[-- Attachment #1: Type: text/plain, Size: 665 bytes --]
On Tue, Oct 29, 2024 at 1:55 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> This hardware can provide only 48 bits of cycle counter. We can leave
> only 24 bits in the cache to extend RX timestamps from 32 bits to 48
> bits. Lower 8 bits of the cached value will be used to check for
> roll-over while extending to full 48 bits.
> This change makes cache writes atomic even on 32 bit platforms and we
> can simply use READ_ONCE()/WRITE_ONCE() pair and remove spinlock. The
> configuration structure will be also reduced by 4 bytes.
>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock
2024-10-29 20:54 ` [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock Vadim Fedorenko
@ 2024-10-30 16:54 ` Michael Chan
0 siblings, 0 replies; 6+ messages in thread
From: Michael Chan @ 2024-10-30 16:54 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Vadim Fedorenko, Pavan Chebbi, Jakub Kicinski, Andrew Lunn,
Paolo Abeni, David S. Miller, netdev, Richard Cochran
[-- Attachment #1: Type: text/plain, Size: 504 bytes --]
On Tue, Oct 29, 2024 at 1:55 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> We can see high contention on ptp_lock while doing RX timestamping
> on high packet rates over several queues. Spinlock is not effecient
> to protect timecounter for RX timestamps when reads are the most
> usual operations and writes are only occasional. It's better to use
> seqlock in such cases.
>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter
2024-10-29 20:54 [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Vadim Fedorenko
2024-10-29 20:54 ` [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock Vadim Fedorenko
2024-10-30 16:53 ` [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Michael Chan
@ 2024-11-03 19:22 ` Jakub Kicinski
2024-11-03 21:17 ` Vadim Fedorenko
2 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2024-11-03 19:22 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Vadim Fedorenko, Michael Chan, Pavan Chebbi, Andrew Lunn,
Paolo Abeni, David S. Miller, netdev, Richard Cochran
On Tue, 29 Oct 2024 13:54:52 -0700 Vadim Fedorenko wrote:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index fa514be87650..820c7e83e586 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -106,7 +106,7 @@ static void bnxt_ptp_get_current_time(struct bnxt *bp)
> if (!ptp)
> return;
> spin_lock_irqsave(&ptp->ptp_lock, flags);
> - WRITE_ONCE(ptp->old_time, ptp->current_time);
> + WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
the casts to u32 seem unnecessary since write to u32 will truncate
the value, anyway, and they make the lines go over 80 columns
> bnxt_refclk_read(bp, NULL, &ptp->current_time);
> spin_unlock_irqrestore(&ptp->ptp_lock, flags);
> }
> @@ -174,7 +174,7 @@ void bnxt_ptp_update_current_time(struct bnxt *bp)
> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>
> bnxt_refclk_read(ptp->bp, NULL, &ptp->current_time);
> - WRITE_ONCE(ptp->old_time, ptp->current_time);
> + WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
> }
>
> static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
> @@ -813,7 +813,7 @@ int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
> if (!ptp)
> return -ENODEV;
>
> - BNXT_READ_TIME64(ptp, time, ptp->old_time);
> + time = (u64)(READ_ONCE(ptp->old_time) << BNXT_HI_TIMER_SHIFT);
And this cast looks misplaced, I presume you want the shift to operate
on 64b. The way this is written the shift will be truncated to 32b,
and then we will promote, with top 32b being all 0.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter
2024-11-03 19:22 ` Jakub Kicinski
@ 2024-11-03 21:17 ` Vadim Fedorenko
0 siblings, 0 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2024-11-03 21:17 UTC (permalink / raw)
To: Jakub Kicinski, Vadim Fedorenko
Cc: Michael Chan, Pavan Chebbi, Andrew Lunn, Paolo Abeni,
David S. Miller, netdev, Richard Cochran
On 03/11/2024 19:22, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 13:54:52 -0700 Vadim Fedorenko wrote:
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index fa514be87650..820c7e83e586 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -106,7 +106,7 @@ static void bnxt_ptp_get_current_time(struct bnxt *bp)
>> if (!ptp)
>> return;
>> spin_lock_irqsave(&ptp->ptp_lock, flags);
>> - WRITE_ONCE(ptp->old_time, ptp->current_time);
>> + WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
>
> the casts to u32 seem unnecessary since write to u32 will truncate
> the value, anyway, and they make the lines go over 80 columns
>
>> bnxt_refclk_read(bp, NULL, &ptp->current_time);
>> spin_unlock_irqrestore(&ptp->ptp_lock, flags);
>> }
>> @@ -174,7 +174,7 @@ void bnxt_ptp_update_current_time(struct bnxt *bp)
>> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>>
>> bnxt_refclk_read(ptp->bp, NULL, &ptp->current_time);
>> - WRITE_ONCE(ptp->old_time, ptp->current_time);
>> + WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
>> }
>>
>> static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
>> @@ -813,7 +813,7 @@ int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
>> if (!ptp)
>> return -ENODEV;
>>
>> - BNXT_READ_TIME64(ptp, time, ptp->old_time);
>> + time = (u64)(READ_ONCE(ptp->old_time) << BNXT_HI_TIMER_SHIFT);
>
> And this cast looks misplaced, I presume you want the shift to operate
> on 64b. The way this is written the shift will be truncated to 32b,
> and then we will promote, with top 32b being all 0.
Indeed, this cast is misplaced. I'll do v5 soon, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-03 21:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-29 20:54 [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Vadim Fedorenko
2024-10-29 20:54 ` [PATCH net-next v4 2/2] bnxt_en: replace PTP spinlock with seqlock Vadim Fedorenko
2024-10-30 16:54 ` Michael Chan
2024-10-30 16:53 ` [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter Michael Chan
2024-11-03 19:22 ` Jakub Kicinski
2024-11-03 21:17 ` Vadim Fedorenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).