This patch-set is to support Ocelot PTP Sync one-step timestamping.
Actually before that, this patch-set cleans up and optimizes the
DSA slave tx timestamp request handling process.
Changes for v2:
- Split tx timestamp optimization patch.
- Updated doc patch.
- Freed skb->cb usage in dsa core driver, and moved to device
drivers.
- Other minor fixes.
Yangbo Lu (7):
net: dsa: check tx timestamp request in core driver
net: dsa: no longer identify PTP packet in core driver
net: dsa: free skb->cb usage in core driver
net: dsa: no longer clone skb in core driver
docs: networking: timestamping: update for DSA switches
net: mscc: ocelot: convert to ocelot_port_txtstamp_request()
net: mscc: ocelot: support PTP Sync one-step timestamping
Documentation/networking/timestamping.rst | 63 ++++++++------
.../net/dsa/hirschmann/hellcreek_hwtstamp.c | 28 ++++---
.../net/dsa/hirschmann/hellcreek_hwtstamp.h | 4 +-
drivers/net/dsa/mv88e6xxx/hwtstamp.c | 26 ++++--
drivers/net/dsa/mv88e6xxx/hwtstamp.h | 10 +--
drivers/net/dsa/ocelot/felix.c | 19 +++--
drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
drivers/net/dsa/sja1105/sja1105_ptp.c | 16 ++--
drivers/net/dsa/sja1105/sja1105_ptp.h | 4 +-
drivers/net/ethernet/mscc/ocelot.c | 83 +++++++++++++++++--
drivers/net/ethernet/mscc/ocelot_net.c | 20 ++---
include/linux/dsa/sja1105.h | 3 +-
include/net/dsa.h | 18 +---
include/soc/mscc/ocelot.h | 21 ++++-
net/dsa/Kconfig | 2 +
net/dsa/slave.c | 23 +----
net/dsa/tag_ocelot.c | 27 +-----
net/dsa/tag_ocelot_8021q.c | 41 +++------
18 files changed, 230 insertions(+), 180 deletions(-)
base-commit: 0ea1041bfa3aa2971f858edd9e05477c2d3d54a0
--
2.25.1
Check tx timestamp request in core driver at very beginning of
dsa_skb_tx_timestamp(), so that most skbs not requiring tx
timestamp just return. And drop such checking in device drivers.
Signed-off-by: Yangbo Lu <[email protected]>
Tested-by: Kurt Kanzenbach <[email protected]>
---
Changes for v2:
- Split from tx timestamp optimization big patch.
---
drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c | 4 ----
drivers/net/dsa/mv88e6xxx/hwtstamp.c | 3 ---
drivers/net/dsa/ocelot/felix.c | 3 +--
net/dsa/slave.c | 3 +++
4 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
index 69dd9a2e8bb6..6ba5e2333066 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
@@ -382,10 +382,6 @@ bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
ps = &hellcreek->ports[port].port_hwtstamp;
- /* Check if the driver is expected to do HW timestamping */
- if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
- return false;
-
/* Make sure the message is a PTP message that needs to be timestamped
* and the interaction with the HW timestamping is enabled. If not, stop
* here
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 094d17a1d037..05ca1d3c6498 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -475,9 +475,6 @@ bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
struct ptp_header *hdr;
- if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
- return false;
-
hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
if (!hdr)
return false;
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 6b5442be0230..1379f86d71ec 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1401,8 +1401,7 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
struct ocelot *ocelot = ds->priv;
struct ocelot_port *ocelot_port = ocelot->ports[port];
- if (ocelot->ptp && (skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
- ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+ if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
ocelot_port_add_txtstamp_skb(ocelot, port, clone);
return true;
}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 77b33bd161b8..b2a802e9330e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -559,6 +559,9 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
struct sk_buff *clone;
unsigned int type;
+ if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+ return;
+
type = ptp_classify_raw(skb);
if (type == PTP_CLASS_NONE)
return;
--
2.25.1
Move ptp_classify_raw out of dsa core driver for handling tx
timestamp request. Let device drivers do this if they want.
Not all drivers want to limit tx timestamping for only PTP
packet.
Signed-off-by: Yangbo Lu <[email protected]>
Tested-by: Kurt Kanzenbach <[email protected]>
---
Changes for v2:
- Split from tx timestamp optimization big patch.
---
drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c | 7 ++++++-
drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h | 2 +-
drivers/net/dsa/mv88e6xxx/hwtstamp.c | 7 ++++++-
drivers/net/dsa/mv88e6xxx/hwtstamp.h | 5 ++---
drivers/net/dsa/ocelot/felix.c | 2 +-
drivers/net/dsa/sja1105/sja1105_ptp.c | 3 +--
drivers/net/dsa/sja1105/sja1105_ptp.h | 2 +-
include/net/dsa.h | 2 +-
net/dsa/slave.c | 12 ++----------
9 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
index 6ba5e2333066..5b2e023468fe 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
@@ -374,14 +374,19 @@ long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
}
bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone, unsigned int type)
+ struct sk_buff *clone)
{
struct hellcreek *hellcreek = ds->priv;
struct hellcreek_port_hwtstamp *ps;
struct ptp_header *hdr;
+ unsigned int type;
ps = &hellcreek->ports[port].port_hwtstamp;
+ type = ptp_classify_raw(clone);
+ if (type == PTP_CLASS_NONE)
+ return false;
+
/* Make sure the message is a PTP message that needs to be timestamped
* and the interaction with the HW timestamping is enabled. If not, stop
* here
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
index c0745ffa1ebb..728cd5dc650f 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
@@ -45,7 +45,7 @@ int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *clone, unsigned int type);
bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone, unsigned int type);
+ struct sk_buff *clone);
int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 05ca1d3c6498..79514a54d903 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -469,11 +469,16 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
}
bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone, unsigned int type)
+ struct sk_buff *clone)
{
struct mv88e6xxx_chip *chip = ds->priv;
struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
struct ptp_header *hdr;
+ unsigned int type;
+
+ type = ptp_classify_raw(clone);
+ if (type == PTP_CLASS_NONE)
+ return false;
hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
if (!hdr)
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index 9da9f197ba02..91fbc7838fc8 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -118,7 +118,7 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *clone, unsigned int type);
bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone, unsigned int type);
+ struct sk_buff *clone);
int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
@@ -152,8 +152,7 @@ static inline bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
}
static inline bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone,
- unsigned int type)
+ struct sk_buff *clone)
{
return false;
}
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 1379f86d71ec..d679f023dc00 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1396,7 +1396,7 @@ static bool felix_rxtstamp(struct dsa_switch *ds, int port,
}
static bool felix_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone, unsigned int type)
+ struct sk_buff *clone)
{
struct ocelot *ocelot = ds->priv;
struct ocelot_port *ocelot_port = ocelot->ports[port];
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 1b90570b257b..72d052de82d8 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -435,8 +435,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
* the skb and have it available in DSA_SKB_CB in the .port_deferred_xmit
* callback, where we will timestamp it synchronously.
*/
-bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *skb, unsigned int type)
+bool sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
struct sja1105_private *priv = ds->priv;
struct sja1105_port *sp = &priv->ports[port];
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
index 3daa33e98e77..c70c4729a06d 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.h
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -105,7 +105,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *skb, unsigned int type);
bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *skb, unsigned int type);
+ struct sk_buff *skb);
int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 507082959aa4..905066055b08 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -741,7 +741,7 @@ struct dsa_switch_ops {
int (*port_hwtstamp_set)(struct dsa_switch *ds, int port,
struct ifreq *ifr);
bool (*port_txtstamp)(struct dsa_switch *ds, int port,
- struct sk_buff *clone, unsigned int type);
+ struct sk_buff *clone);
bool (*port_rxtstamp)(struct dsa_switch *ds, int port,
struct sk_buff *skb, unsigned int type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b2a802e9330e..acaa52e60d7f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -20,7 +20,6 @@
#include <linux/if_bridge.h>
#include <linux/if_hsr.h>
#include <linux/netpoll.h>
-#include <linux/ptp_classify.h>
#include "dsa_priv.h"
@@ -557,15 +556,10 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
{
struct dsa_switch *ds = p->dp->ds;
struct sk_buff *clone;
- unsigned int type;
if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
return;
- type = ptp_classify_raw(skb);
- if (type == PTP_CLASS_NONE)
- return;
-
if (!ds->ops->port_txtstamp)
return;
@@ -573,7 +567,7 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
if (!clone)
return;
- if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type)) {
+ if (ds->ops->port_txtstamp(ds, p->dp->index, clone)) {
DSA_SKB_CB(skb)->clone = clone;
return;
}
@@ -632,9 +626,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
DSA_SKB_CB(skb)->clone = NULL;
- /* Identify PTP protocol packets, clone them, and pass them to the
- * switch driver
- */
+ /* Handle tx timestamp if any */
dsa_skb_tx_timestamp(p, skb);
if (dsa_realloc_skb(skb, dev)) {
--
2.25.1
Free skb->cb usage in core driver and let device drivers decide to
use or not. The reason having a DSA_SKB_CB(skb)->clone was because
dsa_skb_tx_timestamp() which may set the clone pointer was called
before p->xmit() which would use the clone if any, and the device
driver has no way to initialize the clone pointer.
Although for now putting memset(skb->cb, 0, 48) at beginning of
dsa_slave_xmit() by this patch is not very good, there is still way
to improve this. Otherwise, some other new features, like one-step
timestamp which needs a flag of skb marked in dsa_skb_tx_timestamp(),
and handles as one-step timestamp in p->xmit() will face same
situation.
Signed-off-by: Yangbo Lu <[email protected]>
---
Changes for v2:
- Added this patch.
---
drivers/net/dsa/ocelot/felix.c | 1 +
drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
drivers/net/dsa/sja1105/sja1105_ptp.c | 4 +++-
drivers/net/ethernet/mscc/ocelot.c | 6 +++---
drivers/net/ethernet/mscc/ocelot_net.c | 2 +-
include/linux/dsa/sja1105.h | 3 ++-
include/net/dsa.h | 14 --------------
include/soc/mscc/ocelot.h | 8 ++++++++
net/dsa/slave.c | 3 +--
net/dsa/tag_ocelot.c | 8 ++++----
net/dsa/tag_ocelot_8021q.c | 8 ++++----
11 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index d679f023dc00..8980d56ee793 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1403,6 +1403,7 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
ocelot_port_add_txtstamp_skb(ocelot, port, clone);
+ OCELOT_SKB_CB(skb)->clone = clone;
return true;
}
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index d9c198ca0197..405024b637d6 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3137,7 +3137,7 @@ static void sja1105_port_deferred_xmit(struct kthread_work *work)
struct sk_buff *skb;
while ((skb = skb_dequeue(&sp->xmit_queue)) != NULL) {
- struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+ struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone;
mutex_lock(&priv->mgmt_lock);
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 72d052de82d8..0832368aaa96 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -432,7 +432,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
}
/* Called from dsa_skb_tx_timestamp. This callback is just to make DSA clone
- * the skb and have it available in DSA_SKB_CB in the .port_deferred_xmit
+ * the skb and have it available in SJA1105_SKB_CB in the .port_deferred_xmit
* callback, where we will timestamp it synchronously.
*/
bool sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
@@ -443,6 +443,8 @@ bool sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
if (!sp->hwts_tx_en)
return false;
+ SJA1105_SKB_CB(skb)->clone = clone;
+
return true;
}
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 8d06ffaf318a..7da2dd1632b1 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -538,8 +538,8 @@ void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
spin_lock(&ocelot_port->ts_id_lock);
skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
- /* Store timestamp ID in cb[0] of sk_buff */
- clone->cb[0] = ocelot_port->ts_id;
+ /* Store timestamp ID in OCELOT_SKB_CB(clone)->ts_id */
+ OCELOT_SKB_CB(clone)->ts_id = ocelot_port->ts_id;
ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;
skb_queue_tail(&ocelot_port->tx_skbs, clone);
@@ -604,7 +604,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
spin_lock_irqsave(&port->tx_skbs.lock, flags);
skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
- if (skb->cb[0] != id)
+ if (OCELOT_SKB_CB(skb)->ts_id != id)
continue;
__skb_unlink(skb, &port->tx_skbs);
skb_match = skb;
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 36f32a4d9b0f..789a5fba146c 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -520,7 +520,7 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
ocelot_port_add_txtstamp_skb(ocelot, port, clone);
- rew_op |= clone->cb[0] << 3;
+ rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
}
}
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index dd93735ae228..1eb84562b311 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -47,11 +47,12 @@ struct sja1105_tagger_data {
};
struct sja1105_skb_cb {
+ struct sk_buff *clone;
u32 meta_tstamp;
};
#define SJA1105_SKB_CB(skb) \
- ((struct sja1105_skb_cb *)DSA_SKB_CB_PRIV(skb))
+ ((struct sja1105_skb_cb *)((skb)->cb))
struct sja1105_port {
u16 subvlan_map[DSA_8021Q_N_SUBVLAN];
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 905066055b08..5685e60cb082 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -117,20 +117,6 @@ struct dsa_netdevice_ops {
#define MODULE_ALIAS_DSA_TAG_DRIVER(__proto) \
MODULE_ALIAS(DSA_TAG_DRIVER_ALIAS __stringify(__proto##_VALUE))
-struct dsa_skb_cb {
- struct sk_buff *clone;
-};
-
-struct __dsa_skb_cb {
- struct dsa_skb_cb cb;
- u8 priv[48 - sizeof(struct dsa_skb_cb)];
-};
-
-#define DSA_SKB_CB(skb) ((struct dsa_skb_cb *)((skb)->cb))
-
-#define DSA_SKB_CB_PRIV(skb) \
- ((void *)(skb)->cb + offsetof(struct __dsa_skb_cb, priv))
-
struct dsa_switch_tree {
struct list_head list;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 68cdc7ceaf4d..f075aaf70eee 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -689,6 +689,14 @@ struct ocelot_policer {
u32 burst; /* bytes */
};
+struct ocelot_skb_cb {
+ struct sk_buff *clone;
+ u8 ts_id;
+};
+
+#define OCELOT_SKB_CB(skb) \
+ ((struct ocelot_skb_cb *)((skb)->cb))
+
#define ocelot_read_ix(ocelot, reg, gi, ri) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
#define ocelot_read_gix(ocelot, reg, gi) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi))
#define ocelot_read_rix(ocelot, reg, ri) __ocelot_read_ix(ocelot, reg, reg##_RSZ * (ri))
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index acaa52e60d7f..2211894c2381 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -568,7 +568,6 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
return;
if (ds->ops->port_txtstamp(ds, p->dp->index, clone)) {
- DSA_SKB_CB(skb)->clone = clone;
return;
}
@@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
dev_sw_netstats_tx_add(dev, 1, skb->len);
- DSA_SKB_CB(skb)->clone = NULL;
+ memset(skb->cb, 0, 48);
/* Handle tx timestamp if any */
dsa_skb_tx_timestamp(p, skb);
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index f9df9cac81c5..1100a16f1032 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -15,11 +15,11 @@ static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
ocelot_port = ocelot->ports[dp->index];
rew_op = ocelot_port->ptp_cmd;
- /* Retrieve timestamp ID populated inside skb->cb[0] of the
- * clone by ocelot_port_add_txtstamp_skb
+ /* Retrieve timestamp ID populated inside OCELOT_SKB_CB(clone)->ts_id
+ * by ocelot_port_add_txtstamp_skb
*/
if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
- rew_op |= clone->cb[0] << 3;
+ rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
ocelot_ifh_set_rew_op(injection, rew_op);
}
@@ -28,7 +28,7 @@ static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
__be32 ifh_prefix, void **ifh)
{
struct dsa_port *dp = dsa_slave_to_port(netdev);
- struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+ struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
struct dsa_switch *ds = dp->ds;
void *injection;
__be32 *prefix;
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index 5f3e8e124a82..a001a7e3f575 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -28,11 +28,11 @@ static struct sk_buff *ocelot_xmit_ptp(struct dsa_port *dp,
ocelot_port = ocelot->ports[port];
rew_op = ocelot_port->ptp_cmd;
- /* Retrieve timestamp ID populated inside skb->cb[0] of the
- * clone by ocelot_port_add_txtstamp_skb
+ /* Retrieve timestamp ID populated inside OCELOT_SKB_CB(clone)->ts_id
+ * by ocelot_port_add_txtstamp_skb
*/
if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
- rew_op |= clone->cb[0] << 3;
+ rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
@@ -46,7 +46,7 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
u16 tx_vid = dsa_8021q_tx_vid(dp->ds, dp->index);
u16 queue_mapping = skb_get_queue_mapping(skb);
u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
- struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+ struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
/* TX timestamping was requested, so inject through MMIO */
if (clone)
--
2.25.1
Update timestamping doc for DSA switches to describe current
implementation accurately. On TX, the skb cloning is no longer
in DSA generic code.
Signed-off-by: Yangbo Lu <[email protected]>
---
Changes for v2:
- Split from tx timestamp optimization big patch.
- Updated the doc.
---
Documentation/networking/timestamping.rst | 63 ++++++++++++++---------
1 file changed, 39 insertions(+), 24 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index f682e88fa87e..7db3985359bc 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -630,30 +630,45 @@ hardware timestamping on it. This is because the SO_TIMESTAMPING API does not
allow the delivery of multiple hardware timestamps for the same packet, so
anybody else except for the DSA switch port must be prevented from doing so.
-In code, DSA provides for most of the infrastructure for timestamping already,
-in generic code: a BPF classifier (``ptp_classify_raw``) is used to identify
-PTP event messages (any other packets, including PTP general messages, are not
-timestamped), and provides two hooks to drivers:
-
-- ``.port_txtstamp()``: The driver is passed a clone of the timestampable skb
- to be transmitted, before actually transmitting it. Typically, a switch will
- have a PTP TX timestamp register (or sometimes a FIFO) where the timestamp
- becomes available. There may be an IRQ that is raised upon this timestamp's
- availability, or the driver might have to poll after invoking
- ``dev_queue_xmit()`` towards the host interface. Either way, in the
- ``.port_txtstamp()`` method, the driver only needs to save the clone for
- later use (when the timestamp becomes available). Each skb is annotated with
- a pointer to its clone, in ``DSA_SKB_CB(skb)->clone``, to ease the driver's
- job of keeping track of which clone belongs to which skb.
-
-- ``.port_rxtstamp()``: The original (and only) timestampable skb is provided
- to the driver, for it to annotate it with a timestamp, if that is immediately
- available, or defer to later. On reception, timestamps might either be
- available in-band (through metadata in the DSA header, or attached in other
- ways to the packet), or out-of-band (through another RX timestamping FIFO).
- Deferral on RX is typically necessary when retrieving the timestamp needs a
- sleepable context. In that case, it is the responsibility of the DSA driver
- to call ``netif_rx_ni()`` on the freshly timestamped skb.
+In the generic layer, DSA provides the following infrastructure for PTP
+timestamping:
+
+- ``.port_txtstamp()``: a hook called prior to the transmission of
+ packets with a hardware TX timestamping request from user space.
+ This is required for two-step timestamping, since the hardware
+ timestamp becomes available after the actual MAC transmission, so the
+ driver must be prepared to correlate the timestamp with the original
+ packet so that it can re-enqueue the packet back into the socket's
+ error queue. To save the packet for when the timestamp becomes
+ available, the driver can call ``skb_clone_sk`` , save the clone pointer
+ in skb->cb and enqueue a tx skb queue. Typically, a switch will have a
+ PTP TX timestamp register (or sometimes a FIFO) where the timestamp
+ becomes available. In case of a FIFO, the hardware might store
+ key-value pairs of PTP sequence ID/message type/domain number and the
+ actual timestamp. To perform the correlation correctly between the
+ packets in a queue waiting for timestamping and the actual timestamps,
+ drivers can use a BPF classifier (``ptp_classify_raw``) to identify
+ the PTP transport type, and ``ptp_parse_header`` to interpret the PTP
+ header fields. There may be an IRQ that is raised upon this
+ timestamp's availability, or the driver might have to poll after
+ invoking ``dev_queue_xmit()`` towards the host interface.
+ One-step TX timestamping do not require packet cloning, since there is
+ no follow-up message required by the PTP protocol (because the
+ TX timestamp is embedded into the packet by the MAC), and therefore
+ user space does not expect the packet annotated with the TX timestamp
+ to be re-enqueued into its socket's error queue.
+
+- ``.port_rxtstamp()``: On RX, the BPF classifier is run by DSA to
+ identify PTP event messages (any other packets, including PTP general
+ messages, are not timestamped). The original (and only) timestampable
+ skb is provided to the driver, for it to annotate it with a timestamp,
+ if that is immediately available, or defer to later. On reception,
+ timestamps might either be available in-band (through metadata in the
+ DSA header, or attached in other ways to the packet), or out-of-band
+ (through another RX timestamping FIFO). Deferral on RX is typically
+ necessary when retrieving the timestamp needs a sleepable context. In
+ that case, it is the responsibility of the DSA driver to call
+ ``netif_rx_ni()`` on the freshly timestamped skb.
3.2.2 Ethernet PHYs
^^^^^^^^^^^^^^^^^^^
--
2.25.1
Although HWTSTAMP_TX_ONESTEP_SYNC existed in ioctl for hardware timestamp
configuration, the PTP Sync one-step timestamping had never been supported.
This patch is to truely support it.
- ocelot_port_txtstamp_request()
This function handles tx timestamp request by storing
ptp_cmd(tx timestamp type) in OCELOT_SKB_CB(skb)->ptp_cmd,
and additionally for two-step timestamp storing ts_id in
OCELOT_SKB_CB(clone)->ptp_cmd.
- ocelot_ptp_rew_op()
During xmit, this function is called to get rew_op (rewriter option) by
checking skb->cb for tx timestamp request, and configure to transmitting.
Non-onestep-Sync packet with one-step timestamp request falls back to use
two-step timestamp.
Signed-off-by: Yangbo Lu <[email protected]>
---
Changes for v2:
- Utilized OCELOT_SKB_CB(skb)->ptp_cmd for timestamp type.
- Fixed build issue.
- Returned u32 for ocelot_ptp_rew_op, and kept only skb
argument.
---
drivers/net/ethernet/mscc/ocelot.c | 53 ++++++++++++++++++++++++++
drivers/net/ethernet/mscc/ocelot_net.c | 8 ++--
include/soc/mscc/ocelot.h | 8 +++-
net/dsa/Kconfig | 2 +
net/dsa/tag_ocelot.c | 27 ++-----------
net/dsa/tag_ocelot_8021q.c | 41 ++++++--------------
6 files changed, 81 insertions(+), 58 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 3ff4cce1ce7d..0c4283319d7f 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -6,6 +6,7 @@
*/
#include <linux/dsa/ocelot.h>
#include <linux/if_bridge.h>
+#include <linux/ptp_classify.h>
#include <soc/mscc/ocelot_vcap.h>
#include "ocelot.h"
#include "ocelot_vcap.h"
@@ -546,6 +547,46 @@ static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
spin_unlock(&ocelot_port->ts_id_lock);
}
+u32 ocelot_ptp_rew_op(struct sk_buff *skb)
+{
+ struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
+ u8 ptp_cmd = OCELOT_SKB_CB(skb)->ptp_cmd;
+ u32 rew_op = 0;
+
+ if (ptp_cmd == IFH_REW_OP_TWO_STEP_PTP && clone) {
+ rew_op = ptp_cmd;
+ rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
+ } else if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) {
+ rew_op = ptp_cmd;
+ }
+
+ return rew_op;
+}
+EXPORT_SYMBOL(ocelot_ptp_rew_op);
+
+static bool ocelot_ptp_is_onestep_sync(struct sk_buff *skb)
+{
+ struct ptp_header *hdr;
+ unsigned int ptp_class;
+ u8 msgtype, twostep;
+
+ ptp_class = ptp_classify_raw(skb);
+ if (ptp_class == PTP_CLASS_NONE)
+ return false;
+
+ hdr = ptp_parse_header(skb, ptp_class);
+ if (!hdr)
+ return false;
+
+ msgtype = ptp_get_msgtype(hdr, ptp_class);
+ twostep = hdr->flag_field[0] & 0x2;
+
+ if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0)
+ return true;
+
+ return false;
+}
+
int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
struct sk_buff *skb,
struct sk_buff **clone)
@@ -553,12 +594,24 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
struct ocelot_port *ocelot_port = ocelot->ports[port];
u8 ptp_cmd = ocelot_port->ptp_cmd;
+ /* Store ptp_cmd in OCELOT_SKB_CB(skb)->ptp_cmd */
+ if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) {
+ if (ocelot_ptp_is_onestep_sync(skb)) {
+ OCELOT_SKB_CB(skb)->ptp_cmd = ptp_cmd;
+ return 0;
+ }
+
+ /* Fall back to two-step timestamping */
+ ptp_cmd = IFH_REW_OP_TWO_STEP_PTP;
+ }
+
if (ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
*clone = skb_clone_sk(skb);
if (!(*clone))
return -ENOMEM;
ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
+ OCELOT_SKB_CB(skb)->ptp_cmd = ptp_cmd;
}
return 0;
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index e99c8fb3cb15..aad33d22c33f 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -514,10 +514,10 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
}
- if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
- rew_op = ocelot_port->ptp_cmd;
- rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
- }
+ if (clone)
+ OCELOT_SKB_CB(skb)->clone = clone;
+
+ rew_op = ocelot_ptp_rew_op(skb);
}
ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index f7632519cb9c..2f5ce4d4fdbf 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -691,6 +691,7 @@ struct ocelot_policer {
struct ocelot_skb_cb {
struct sk_buff *clone;
+ u8 ptp_cmd;
u8 ts_id;
};
@@ -748,15 +749,16 @@ u32 __ocelot_target_read_ix(struct ocelot *ocelot, enum ocelot_target target,
void __ocelot_target_write_ix(struct ocelot *ocelot, enum ocelot_target target,
u32 val, u32 reg, u32 offset);
-/* Packet I/O */
#if IS_ENABLED(CONFIG_MSCC_OCELOT_SWITCH_LIB)
+/* Packet I/O */
bool ocelot_can_inject(struct ocelot *ocelot, int grp);
void ocelot_port_inject_frame(struct ocelot *ocelot, int port, int grp,
u32 rew_op, struct sk_buff *skb);
int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **skb);
void ocelot_drain_cpu_queue(struct ocelot *ocelot, int grp);
+u32 ocelot_ptp_rew_op(struct sk_buff *skb);
#else
static inline bool ocelot_can_inject(struct ocelot *ocelot, int grp)
@@ -780,6 +782,10 @@ static inline void ocelot_drain_cpu_queue(struct ocelot *ocelot, int grp)
{
}
+static inline u32 ocelot_ptp_rew_op(struct sk_buff *skb)
+{
+ return 0;
+}
#endif
/* Hardware initialization */
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index cbc2bd643ab2..5baba7021427 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -111,6 +111,8 @@ config NET_DSA_TAG_RTL4_A
config NET_DSA_TAG_OCELOT
tristate "Tag driver for Ocelot family of switches, using NPI port"
+ depends on MSCC_OCELOT_SWITCH_LIB || \
+ (MSCC_OCELOT_SWITCH_LIB=n && COMPILE_TEST)
select PACKING
help
Say Y or M if you want to enable NPI tagging for the Ocelot switches
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index 1100a16f1032..91f0fd1242cd 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -5,33 +5,14 @@
#include <soc/mscc/ocelot.h>
#include "dsa_priv.h"
-static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
- struct sk_buff *clone)
-{
- struct ocelot *ocelot = dp->ds->priv;
- struct ocelot_port *ocelot_port;
- u64 rew_op;
-
- ocelot_port = ocelot->ports[dp->index];
- rew_op = ocelot_port->ptp_cmd;
-
- /* Retrieve timestamp ID populated inside OCELOT_SKB_CB(clone)->ts_id
- * by ocelot_port_add_txtstamp_skb
- */
- if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
- rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
-
- ocelot_ifh_set_rew_op(injection, rew_op);
-}
-
static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
__be32 ifh_prefix, void **ifh)
{
struct dsa_port *dp = dsa_slave_to_port(netdev);
- struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
struct dsa_switch *ds = dp->ds;
void *injection;
__be32 *prefix;
+ u32 rew_op = 0;
injection = skb_push(skb, OCELOT_TAG_LEN);
prefix = skb_push(skb, OCELOT_SHORT_PREFIX_LEN);
@@ -42,9 +23,9 @@ static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
ocelot_ifh_set_src(injection, ds->num_ports);
ocelot_ifh_set_qos_class(injection, skb->priority);
- /* TX timestamping was requested */
- if (clone)
- ocelot_xmit_ptp(dp, injection, clone);
+ rew_op = ocelot_ptp_rew_op(skb);
+ if (rew_op)
+ ocelot_ifh_set_rew_op(injection, rew_op);
*ifh = injection;
}
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index a001a7e3f575..62a93303bd63 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -13,32 +13,6 @@
#include <soc/mscc/ocelot_ptp.h>
#include "dsa_priv.h"
-static struct sk_buff *ocelot_xmit_ptp(struct dsa_port *dp,
- struct sk_buff *skb,
- struct sk_buff *clone)
-{
- struct ocelot *ocelot = dp->ds->priv;
- struct ocelot_port *ocelot_port;
- int port = dp->index;
- u32 rew_op;
-
- if (!ocelot_can_inject(ocelot, 0))
- return NULL;
-
- ocelot_port = ocelot->ports[port];
- rew_op = ocelot_port->ptp_cmd;
-
- /* Retrieve timestamp ID populated inside OCELOT_SKB_CB(clone)->ts_id
- * by ocelot_port_add_txtstamp_skb
- */
- if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
- rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
-
- ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
-
- return NULL;
-}
-
static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
struct net_device *netdev)
{
@@ -46,11 +20,18 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
u16 tx_vid = dsa_8021q_tx_vid(dp->ds, dp->index);
u16 queue_mapping = skb_get_queue_mapping(skb);
u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
- struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
+ struct ocelot *ocelot = dp->ds->priv;
+ int port = dp->index;
+ u32 rew_op = 0;
+
+ rew_op = ocelot_ptp_rew_op(skb);
+ if (rew_op) {
+ if (!ocelot_can_inject(ocelot, 0))
+ return NULL;
- /* TX timestamping was requested, so inject through MMIO */
- if (clone)
- return ocelot_xmit_ptp(dp, skb, clone);
+ ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
+ return NULL;
+ }
return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,
((pcp << VLAN_PRIO_SHIFT) | tx_vid));
--
2.25.1
Convert to a common ocelot_port_txtstamp_request() for TX timestamp
request handling.
Signed-off-by: Yangbo Lu <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
Changes for v2:
- Rebased.
---
drivers/net/dsa/ocelot/felix.c | 15 +++++++--------
drivers/net/ethernet/mscc/ocelot.c | 24 +++++++++++++++++++++---
drivers/net/ethernet/mscc/ocelot_net.c | 18 +++++++-----------
include/soc/mscc/ocelot.h | 5 +++--
4 files changed, 38 insertions(+), 24 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index b28280b6e91a..ce607fbaaa3a 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1399,17 +1399,16 @@ static void felix_txtstamp(struct dsa_switch *ds, int port,
struct sk_buff *skb)
{
struct ocelot *ocelot = ds->priv;
- struct ocelot_port *ocelot_port = ocelot->ports[port];
- struct sk_buff *clone;
+ struct sk_buff *clone = NULL;
- if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
- clone = skb_clone_sk(skb);
- if (!clone)
- return;
+ if (!ocelot->ptp)
+ return;
- ocelot_port_add_txtstamp_skb(ocelot, port, clone);
+ if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone))
+ return;
+
+ if (clone)
OCELOT_SKB_CB(skb)->clone = clone;
- }
}
static int felix_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 7da2dd1632b1..3ff4cce1ce7d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -530,8 +530,8 @@ void ocelot_port_disable(struct ocelot *ocelot, int port)
}
EXPORT_SYMBOL(ocelot_port_disable);
-void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
- struct sk_buff *clone)
+static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
+ struct sk_buff *clone)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
@@ -545,7 +545,25 @@ void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
spin_unlock(&ocelot_port->ts_id_lock);
}
-EXPORT_SYMBOL(ocelot_port_add_txtstamp_skb);
+
+int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
+ struct sk_buff *skb,
+ struct sk_buff **clone)
+{
+ struct ocelot_port *ocelot_port = ocelot->ports[port];
+ u8 ptp_cmd = ocelot_port->ptp_cmd;
+
+ if (ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+ *clone = skb_clone_sk(skb);
+ if (!(*clone))
+ return -ENOMEM;
+
+ ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(ocelot_port_txtstamp_request);
static void ocelot_get_hwtimestamp(struct ocelot *ocelot,
struct timespec64 *ts)
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 789a5fba146c..e99c8fb3cb15 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -507,19 +507,15 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
/* Check if timestamping is needed */
if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
- rew_op = ocelot_port->ptp_cmd;
+ struct sk_buff *clone = NULL;
- if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
- struct sk_buff *clone;
-
- clone = skb_clone_sk(skb);
- if (!clone) {
- kfree_skb(skb);
- return NETDEV_TX_OK;
- }
-
- ocelot_port_add_txtstamp_skb(ocelot, port, clone);
+ if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone)) {
+ kfree_skb(skb);
+ return NETDEV_TX_OK;
+ }
+ if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+ rew_op = ocelot_port->ptp_cmd;
rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
}
}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index f075aaf70eee..f7632519cb9c 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -828,8 +828,9 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid);
int ocelot_hwstamp_get(struct ocelot *ocelot, int port, struct ifreq *ifr);
int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr);
-void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
- struct sk_buff *clone);
+int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
+ struct sk_buff *skb,
+ struct sk_buff **clone);
void ocelot_get_txtstamp(struct ocelot *ocelot);
void ocelot_port_set_maxlen(struct ocelot *ocelot, int port, size_t sdu);
int ocelot_get_max_mtu(struct ocelot *ocelot, int port);
--
2.25.1
It was a waste to clone skb directly in dsa_skb_tx_timestamp().
For one-step timestamping, a clone was not needed. For any failure of
port_txtstamp (this may usually happen), the skb clone had to be freed.
So this patch moves skb cloning for tx timestamp out of dsa core, and
let drivers clone skb in port_txtstamp if they really need.
Signed-off-by: Yangbo Lu <[email protected]>
Tested-by: Kurt Kanzenbach <[email protected]>
---
Changes for v2:
- Split from tx timestamp optimization big patch.
- Returned void type for port_txtstamp.
---
.../net/dsa/hirschmann/hellcreek_hwtstamp.c | 25 +++++++++++--------
.../net/dsa/hirschmann/hellcreek_hwtstamp.h | 4 +--
drivers/net/dsa/mv88e6xxx/hwtstamp.c | 24 +++++++++++-------
drivers/net/dsa/mv88e6xxx/hwtstamp.h | 9 +++----
drivers/net/dsa/ocelot/felix.c | 12 +++++----
drivers/net/dsa/sja1105/sja1105_ptp.c | 13 ++++++----
drivers/net/dsa/sja1105/sja1105_ptp.h | 2 +-
include/net/dsa.h | 4 +--
net/dsa/slave.c | 11 +-------
9 files changed, 55 insertions(+), 49 deletions(-)
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
index 5b2e023468fe..40b41c794dfa 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
@@ -373,31 +373,38 @@ long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
return restart ? 1 : -1;
}
-bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone)
+void hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb)
{
struct hellcreek *hellcreek = ds->priv;
struct hellcreek_port_hwtstamp *ps;
struct ptp_header *hdr;
+ struct sk_buff *clone;
unsigned int type;
ps = &hellcreek->ports[port].port_hwtstamp;
- type = ptp_classify_raw(clone);
+ type = ptp_classify_raw(skb);
if (type == PTP_CLASS_NONE)
- return false;
+ return;
/* Make sure the message is a PTP message that needs to be timestamped
* and the interaction with the HW timestamping is enabled. If not, stop
* here
*/
- hdr = hellcreek_should_tstamp(hellcreek, port, clone, type);
+ hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
if (!hdr)
- return false;
+ return;
+
+ clone = skb_clone_sk(skb);
+ if (!clone)
+ return;
if (test_and_set_bit_lock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
- &ps->state))
- return false;
+ &ps->state)) {
+ kfree_skb(clone);
+ return;
+ }
ps->tx_skb = clone;
@@ -407,8 +414,6 @@ bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
ps->tx_tstamp_start = jiffies;
ptp_schedule_worker(hellcreek->ptp_clock, 0);
-
- return true;
}
bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
index 728cd5dc650f..71af77efb28b 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
@@ -44,8 +44,8 @@ int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *clone, unsigned int type);
-bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone);
+void hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb);
int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 79514a54d903..8f74ffc7a279 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -468,32 +468,38 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
return restart ? 1 : -1;
}
-bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone)
+void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb)
{
struct mv88e6xxx_chip *chip = ds->priv;
struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
struct ptp_header *hdr;
+ struct sk_buff *clone;
unsigned int type;
- type = ptp_classify_raw(clone);
+ type = ptp_classify_raw(skb);
if (type == PTP_CLASS_NONE)
- return false;
+ return;
- hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
+ hdr = mv88e6xxx_should_tstamp(chip, port, skb, type);
if (!hdr)
- return false;
+ return;
+
+ clone = skb_clone_sk(skb);
+ if (!clone)
+ return;
if (test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
- &ps->state))
- return false;
+ &ps->state)) {
+ kfree_skb(clone);
+ return;
+ }
ps->tx_skb = clone;
ps->tx_tstamp_start = jiffies;
ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
ptp_schedule_worker(chip->ptp_clock, 0);
- return true;
}
int mv88e6165_global_disable(struct mv88e6xxx_chip *chip)
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index 91fbc7838fc8..cf7fb6d660b1 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -117,8 +117,8 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *clone, unsigned int type);
-bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone);
+void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb);
int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
@@ -151,10 +151,9 @@ static inline bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
return false;
}
-static inline bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone)
+static inline void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb)
{
- return false;
}
static inline int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 8980d56ee793..b28280b6e91a 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1395,19 +1395,21 @@ static bool felix_rxtstamp(struct dsa_switch *ds, int port,
return false;
}
-static bool felix_txtstamp(struct dsa_switch *ds, int port,
- struct sk_buff *clone)
+static void felix_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb)
{
struct ocelot *ocelot = ds->priv;
struct ocelot_port *ocelot_port = ocelot->ports[port];
+ struct sk_buff *clone;
if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+ clone = skb_clone_sk(skb);
+ if (!clone)
+ return;
+
ocelot_port_add_txtstamp_skb(ocelot, port, clone);
OCELOT_SKB_CB(skb)->clone = clone;
- return true;
}
-
- return false;
}
static int felix_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 0832368aaa96..0bc566b9e958 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -431,21 +431,24 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
return true;
}
-/* Called from dsa_skb_tx_timestamp. This callback is just to make DSA clone
+/* Called from dsa_skb_tx_timestamp. This callback is just to clone
* the skb and have it available in SJA1105_SKB_CB in the .port_deferred_xmit
* callback, where we will timestamp it synchronously.
*/
-bool sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
+void sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
struct sja1105_private *priv = ds->priv;
struct sja1105_port *sp = &priv->ports[port];
+ struct sk_buff *clone;
if (!sp->hwts_tx_en)
- return false;
+ return;
- SJA1105_SKB_CB(skb)->clone = clone;
+ clone = skb_clone_sk(skb);
+ if (!clone)
+ return;
- return true;
+ SJA1105_SKB_CB(skb)->clone = clone;
}
static int sja1105_ptp_reset(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
index c70c4729a06d..34f97f58a355 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.h
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -104,7 +104,7 @@ void sja1105_ptp_txtstamp_skb(struct dsa_switch *ds, int slot,
bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *skb, unsigned int type);
-bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
+void sja1105_port_txtstamp(struct dsa_switch *ds, int port,
struct sk_buff *skb);
int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 5685e60cb082..e1a2610a0e06 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -726,8 +726,8 @@ struct dsa_switch_ops {
struct ifreq *ifr);
int (*port_hwtstamp_set)(struct dsa_switch *ds, int port,
struct ifreq *ifr);
- bool (*port_txtstamp)(struct dsa_switch *ds, int port,
- struct sk_buff *clone);
+ void (*port_txtstamp)(struct dsa_switch *ds, int port,
+ struct sk_buff *skb);
bool (*port_rxtstamp)(struct dsa_switch *ds, int port,
struct sk_buff *skb, unsigned int type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2211894c2381..2033d8bac23d 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -555,7 +555,6 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
struct sk_buff *skb)
{
struct dsa_switch *ds = p->dp->ds;
- struct sk_buff *clone;
if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
return;
@@ -563,15 +562,7 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
if (!ds->ops->port_txtstamp)
return;
- clone = skb_clone_sk(skb);
- if (!clone)
- return;
-
- if (ds->ops->port_txtstamp(ds, p->dp->index, clone)) {
- return;
- }
-
- kfree_skb(clone);
+ ds->ops->port_txtstamp(ds, p->dp->index, skb);
}
netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev)
--
2.25.1
On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>
> dev_sw_netstats_tx_add(dev, 1, skb->len);
>
> - DSA_SKB_CB(skb)->clone = NULL;
> + memset(skb->cb, 0, 48);
Replace hard coded 48 with sizeof() please.
Thanks,
Richard
On Mon, Apr 26, 2021 at 05:37:55PM +0800, Yangbo Lu wrote:
> This patch-set is to support Ocelot PTP Sync one-step timestamping.
> Actually before that, this patch-set cleans up and optimizes the
> DSA slave tx timestamp request handling process.
>
> Changes for v2:
> - Split tx timestamp optimization patch.
> - Updated doc patch.
> - Freed skb->cb usage in dsa core driver, and moved to device
> drivers.
> - Other minor fixes.
>
> Yangbo Lu (7):
> net: dsa: check tx timestamp request in core driver
> net: dsa: no longer identify PTP packet in core driver
> net: dsa: free skb->cb usage in core driver
> net: dsa: no longer clone skb in core driver
> docs: networking: timestamping: update for DSA switches
> net: mscc: ocelot: convert to ocelot_port_txtstamp_request()
> net: mscc: ocelot: support PTP Sync one-step timestamping
Please fix the memset. Then, for the series:
Acked-by: Richard Cochran <[email protected]>
On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> Free skb->cb usage in core driver and let device drivers decide to
> use or not. The reason having a DSA_SKB_CB(skb)->clone was because
> dsa_skb_tx_timestamp() which may set the clone pointer was called
> before p->xmit() which would use the clone if any, and the device
> driver has no way to initialize the clone pointer.
>
> Although for now putting memset(skb->cb, 0, 48) at beginning of
> dsa_slave_xmit() by this patch is not very good, there is still way
> to improve this. Otherwise, some other new features, like one-step
> timestamp which needs a flag of skb marked in dsa_skb_tx_timestamp(),
> and handles as one-step timestamp in p->xmit() will face same
> situation.
>
> Signed-off-by: Yangbo Lu <[email protected]>
> ---
> Changes for v2:
> - Added this patch.
> ---
> drivers/net/dsa/ocelot/felix.c | 1 +
> drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
> drivers/net/dsa/sja1105/sja1105_ptp.c | 4 +++-
> drivers/net/ethernet/mscc/ocelot.c | 6 +++---
> drivers/net/ethernet/mscc/ocelot_net.c | 2 +-
> include/linux/dsa/sja1105.h | 3 ++-
> include/net/dsa.h | 14 --------------
> include/soc/mscc/ocelot.h | 8 ++++++++
> net/dsa/slave.c | 3 +--
> net/dsa/tag_ocelot.c | 8 ++++----
> net/dsa/tag_ocelot_8021q.c | 8 ++++----
> 11 files changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index d679f023dc00..8980d56ee793 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -1403,6 +1403,7 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
>
> if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> + OCELOT_SKB_CB(skb)->clone = clone;
> return true;
> }
>
Uh-oh, this patch fails to build:
In file included from ./include/soc/mscc/ocelot_vcap.h:9:0,
from drivers/net/dsa/ocelot/felix.c:9:
drivers/net/dsa/ocelot/felix.c: In function ‘felix_txtstamp’:
drivers/net/dsa/ocelot/felix.c:1406:17: error: ‘skb’ undeclared (first use in this function)
OCELOT_SKB_CB(skb)->clone = clone;
^
./include/soc/mscc/ocelot.h:698:29: note: in definition of macro ‘OCELOT_SKB_CB’
((struct ocelot_skb_cb *)((skb)->cb))
^~~
drivers/net/dsa/ocelot/felix.c:1406:17: note: each undeclared identifier is reported only once for each function it appears in
OCELOT_SKB_CB(skb)->clone = clone;
^
./include/soc/mscc/ocelot.h:698:29: note: in definition of macro ‘OCELOT_SKB_CB’
((struct ocelot_skb_cb *)((skb)->cb))
^~~
It depends on changes made in patch 3.
On 4/26/21 2:37 AM, Yangbo Lu wrote:
> Check tx timestamp request in core driver at very beginning of
> dsa_skb_tx_timestamp(), so that most skbs not requiring tx
> timestamp just return. And drop such checking in device drivers.
>
> Signed-off-by: Yangbo Lu <[email protected]>
> Tested-by: Kurt Kanzenbach <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Mon, Apr 26, 2021 at 06:38:46AM -0700, Richard Cochran wrote:
> On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> > @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
> >
> > dev_sw_netstats_tx_add(dev, 1, skb->len);
> >
> > - DSA_SKB_CB(skb)->clone = NULL;
> > + memset(skb->cb, 0, 48);
>
> Replace hard coded 48 with sizeof() please.
You mean just a trivial change like this, right?
memset(skb->cb, 0, sizeof(skb->cb));
And not what I had suggested in v1, which would have looked something
like this:
-----------------------------[cut here]-----------------------------
diff --git a/include/net/dsa.h b/include/net/dsa.h
index e1a2610a0e06..c75b249e846f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -92,6 +92,7 @@ struct dsa_device_ops {
*/
bool (*filter)(const struct sk_buff *skb, struct net_device *dev);
unsigned int overhead;
+ unsigned int skb_cb_size;
const char *name;
enum dsa_tag_protocol proto;
/* Some tagging protocols either mangle or shift the destination MAC
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2033d8bac23d..2230596b48b7 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -610,11 +610,14 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev)
static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
+ const struct dsa_device_ops *tag_ops;
struct sk_buff *nskb;
dev_sw_netstats_tx_add(dev, 1, skb->len);
- memset(skb->cb, 0, 48);
+ tag_ops = p->dp->cpu_dp->tag_ops;
+ if (tag_ops->skb_cb_size)
+ memset(skb->cb, 0, tag_ops->skb_cb_size);
/* Handle tx timestamp if any */
dsa_skb_tx_timestamp(p, skb);
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 50496013cdb7..1b337fa104dc 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -365,6 +365,7 @@ static const struct dsa_device_ops sja1105_netdev_ops = {
.overhead = VLAN_HLEN,
.flow_dissect = sja1105_flow_dissect,
.promisc_on_master = true,
+ .skb_cb_size = sizeof(struct sja1105_skb_cb),
};
MODULE_LICENSE("GPL v2");
-----------------------------[cut here]-----------------------------
I wanted to see how badly impacted would the performance be, so I
created an IPv4 forwarding setup on the NXP LS1021A-TSN board (gianfar +
sja1105):
#!/bin/bash
ETH0=swp3
ETH1=swp2
systemctl stop ptp4l # runs a BPF classifier on every packet
systemctl stop phc2sys
echo 1 > /proc/sys/net/ipv4/ip_forward
ip addr flush $ETH0 && ip addr add 192.168.100.1/24 dev $ETH0 && ip link set $ETH0 up
ip addr flush $ETH1 && ip addr add 192.168.200.1/24 dev $ETH1 && ip link set $ETH1 up
arp -s 192.168.100.2 00:04:9f:06:00:09 dev $ETH0
arp -s 192.168.200.2 00:04:9f:06:00:0a dev $ETH1
ethtool --config-nfc eth2 flow-type ether dst 00:1f:7b:63:01:d4 m ff:ff:ff:ff:ff:ff action 0
and I got the following results on 1 CPU, 64B UDP packets (yes, I know
the baseline results suck, I haven't investigated why that is, but
nonetheless, it should still be relevant as far as comparative results
go):
Unpatched net-next:
proto 17: 65695 pkt/s
proto 17: 65725 pkt/s
proto 17: 65732 pkt/s
proto 17: 65720 pkt/s
proto 17: 65695 pkt/s
proto 17: 65725 pkt/s
proto 17: 65732 pkt/s
proto 17: 65720 pkt/s
After patch 1:
proto 17: 72679 pkt/s
proto 17: 72677 pkt/s
proto 17: 72669 pkt/s
proto 17: 72707 pkt/s
proto 17: 72696 pkt/s
proto 17: 72699 pkt/s
After patch 2:
proto 17: 72292 pkt/s
proto 17: 72425 pkt/s
proto 17: 72485 pkt/s
proto 17: 72478 pkt/s
After patch 4 (as 3 doesn't build):
proto 17: 72437 pkt/s
proto 17: 72510 pkt/s
proto 17: 72479 pkt/s
proto 17: 72499 pkt/s
proto 17: 72497 pkt/s
proto 17: 72427 pkt/s
With the change I pasted above:
proto 17: 71891 pkt/s
proto 17: 71810 pkt/s
proto 17: 71850 pkt/s
proto 17: 71826 pkt/s
proto 17: 71798 pkt/s
proto 17: 71786 pkt/s
proto 17: 71814 pkt/s
proto 17: 71814 pkt/s
proto 17: 72010 pkt/s
So basically, not only are we better off just zero-initializing the
complete skb->cb instead of looking up the tagger's skb_cb_size, but
zero-initializing the skb->cb isn't even all that bad. Yangbo's change
is an overall win anyway, all things considered. So just change the
memset as Richard suggested, make sure all patches compile, and we
should be good to go.
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: 2021年4月27日 2:17
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; Richard Cochran <[email protected]>;
> Vladimir Oltean <[email protected]>; David S . Miller
> <[email protected]>; Jakub Kicinski <[email protected]>; Jonathan Corbet
> <[email protected]>; Kurt Kanzenbach <[email protected]>; Andrew Lunn
> <[email protected]>; Vivien Didelot <[email protected]>; Florian
> Fainelli <[email protected]>; Claudiu Manoil <[email protected]>;
> Alexandre Belloni <[email protected]>;
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver
>
> On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> > Free skb->cb usage in core driver and let device drivers decide to use
> > or not. The reason having a DSA_SKB_CB(skb)->clone was because
> > dsa_skb_tx_timestamp() which may set the clone pointer was called
> > before p->xmit() which would use the clone if any, and the device
> > driver has no way to initialize the clone pointer.
> >
> > Although for now putting memset(skb->cb, 0, 48) at beginning of
> > dsa_slave_xmit() by this patch is not very good, there is still way to
> > improve this. Otherwise, some other new features, like one-step
> > timestamp which needs a flag of skb marked in dsa_skb_tx_timestamp(),
> > and handles as one-step timestamp in p->xmit() will face same
> > situation.
> >
> > Signed-off-by: Yangbo Lu <[email protected]>
> > ---
> > Changes for v2:
> > - Added this patch.
> > ---
> > drivers/net/dsa/ocelot/felix.c | 1 +
> > drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
> > drivers/net/dsa/sja1105/sja1105_ptp.c | 4 +++-
> > drivers/net/ethernet/mscc/ocelot.c | 6 +++---
> > drivers/net/ethernet/mscc/ocelot_net.c | 2 +-
> > include/linux/dsa/sja1105.h | 3 ++-
> > include/net/dsa.h | 14 --------------
> > include/soc/mscc/ocelot.h | 8 ++++++++
> > net/dsa/slave.c | 3 +--
> > net/dsa/tag_ocelot.c | 8 ++++----
> > net/dsa/tag_ocelot_8021q.c | 8 ++++----
> > 11 files changed, 28 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/dsa/ocelot/felix.c
> > b/drivers/net/dsa/ocelot/felix.c index d679f023dc00..8980d56ee793
> > 100644
> > --- a/drivers/net/dsa/ocelot/felix.c
> > +++ b/drivers/net/dsa/ocelot/felix.c
> > @@ -1403,6 +1403,7 @@ static bool felix_txtstamp(struct dsa_switch
> > *ds, int port,
> >
> > if (ocelot->ptp && ocelot_port->ptp_cmd ==
> IFH_REW_OP_TWO_STEP_PTP) {
> > ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> > + OCELOT_SKB_CB(skb)->clone = clone;
> > return true;
> > }
> >
>
> Uh-oh, this patch fails to build:
>
> In file included from ./include/soc/mscc/ocelot_vcap.h:9:0,
> from drivers/net/dsa/ocelot/felix.c:9:
> drivers/net/dsa/ocelot/felix.c: In function ‘felix_txtstamp’:
> drivers/net/dsa/ocelot/felix.c:1406:17: error: ‘skb’ undeclared (first use in
> this function)
> OCELOT_SKB_CB(skb)->clone = clone;
> ^
> ./include/soc/mscc/ocelot.h:698:29: note: in definition of macro
> ‘OCELOT_SKB_CB’
> ((struct ocelot_skb_cb *)((skb)->cb))
> ^~~
> drivers/net/dsa/ocelot/felix.c:1406:17: note: each undeclared identifier is
> reported only once for each function it appears in
> OCELOT_SKB_CB(skb)->clone = clone;
> ^
> ./include/soc/mscc/ocelot.h:698:29: note: in definition of macro
> ‘OCELOT_SKB_CB’
> ((struct ocelot_skb_cb *)((skb)->cb))
> ^~~
>
> It depends on changes made in patch 3.
Oh.. That's a sequence problem.
I switched patch #3 and patch #4 with rebasing to fix up in v3 version.
Thanks.
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: 2021??4??27?? 2:40
> To: Richard Cochran <[email protected]>
> Cc: Y.b. Lu <[email protected]>; [email protected]; Vladimir Oltean
> <[email protected]>; David S . Miller <[email protected]>; Jakub
> Kicinski <[email protected]>; Jonathan Corbet <[email protected]>; Kurt
> Kanzenbach <[email protected]>; Andrew Lunn <[email protected]>; Vivien
> Didelot <[email protected]>; Florian Fainelli <[email protected]>;
> Claudiu Manoil <[email protected]>; Alexandre Belloni
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver
>
> On Mon, Apr 26, 2021 at 06:38:46AM -0700, Richard Cochran wrote:
> > On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> > > @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff
> > > *skb, struct net_device *dev)
> > >
> > > dev_sw_netstats_tx_add(dev, 1, skb->len);
> > >
> > > - DSA_SKB_CB(skb)->clone = NULL;
> > > + memset(skb->cb, 0, 48);
> >
> > Replace hard coded 48 with sizeof() please.
>
> You mean just a trivial change like this, right?
>
> memset(skb->cb, 0, sizeof(skb->cb));
>
> And not what I had suggested in v1, which would have looked something like
> this:
>
> -----------------------------[cut here]-----------------------------
> diff --git a/include/net/dsa.h b/include/net/dsa.h index
> e1a2610a0e06..c75b249e846f 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -92,6 +92,7 @@ struct dsa_device_ops {
> */
> bool (*filter)(const struct sk_buff *skb, struct net_device *dev);
> unsigned int overhead;
> + unsigned int skb_cb_size;
> const char *name;
> enum dsa_tag_protocol proto;
> /* Some tagging protocols either mangle or shift the destination MAC diff
> --git a/net/dsa/slave.c b/net/dsa/slave.c index 2033d8bac23d..2230596b48b7
> 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -610,11 +610,14 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct
> net_device *dev) static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb,
> struct net_device *dev) {
> struct dsa_slave_priv *p = netdev_priv(dev);
> + const struct dsa_device_ops *tag_ops;
> struct sk_buff *nskb;
>
> dev_sw_netstats_tx_add(dev, 1, skb->len);
>
> - memset(skb->cb, 0, 48);
> + tag_ops = p->dp->cpu_dp->tag_ops;
> + if (tag_ops->skb_cb_size)
> + memset(skb->cb, 0, tag_ops->skb_cb_size);
>
> /* Handle tx timestamp if any */
> dsa_skb_tx_timestamp(p, skb);
> diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c index
> 50496013cdb7..1b337fa104dc 100644
> --- a/net/dsa/tag_sja1105.c
> +++ b/net/dsa/tag_sja1105.c
> @@ -365,6 +365,7 @@ static const struct dsa_device_ops
> sja1105_netdev_ops = {
> .overhead = VLAN_HLEN,
> .flow_dissect = sja1105_flow_dissect,
> .promisc_on_master = true,
> + .skb_cb_size = sizeof(struct sja1105_skb_cb),
> };
>
> MODULE_LICENSE("GPL v2");
> -----------------------------[cut here]-----------------------------
>
> I wanted to see how badly impacted would the performance be, so I created
> an IPv4 forwarding setup on the NXP LS1021A-TSN board (gianfar +
> sja1105):
>
> #!/bin/bash
>
> ETH0=swp3
> ETH1=swp2
>
> systemctl stop ptp4l # runs a BPF classifier on every packet systemctl stop
> phc2sys
>
> echo 1 > /proc/sys/net/ipv4/ip_forward
> ip addr flush $ETH0 && ip addr add 192.168.100.1/24 dev $ETH0 && ip link
> set $ETH0 up ip addr flush $ETH1 && ip addr add 192.168.200.1/24 dev $ETH1
> && ip link set $ETH1 up
>
> arp -s 192.168.100.2 00:04:9f:06:00:09 dev $ETH0 arp -s 192.168.200.2
> 00:04:9f:06:00:0a dev $ETH1
>
> ethtool --config-nfc eth2 flow-type ether dst 00:1f:7b:63:01:d4 m ff:ff:ff:ff:ff:ff
> action 0
>
> and I got the following results on 1 CPU, 64B UDP packets (yes, I know the
> baseline results suck, I haven't investigated why that is, but nonetheless, it
> should still be relevant as far as comparative results
> go):
>
> Unpatched net-next:
> proto 17: 65695 pkt/s
> proto 17: 65725 pkt/s
> proto 17: 65732 pkt/s
> proto 17: 65720 pkt/s
> proto 17: 65695 pkt/s
> proto 17: 65725 pkt/s
> proto 17: 65732 pkt/s
> proto 17: 65720 pkt/s
>
>
> After patch 1:
> proto 17: 72679 pkt/s
> proto 17: 72677 pkt/s
> proto 17: 72669 pkt/s
> proto 17: 72707 pkt/s
> proto 17: 72696 pkt/s
> proto 17: 72699 pkt/s
>
> After patch 2:
> proto 17: 72292 pkt/s
> proto 17: 72425 pkt/s
> proto 17: 72485 pkt/s
> proto 17: 72478 pkt/s
>
> After patch 4 (as 3 doesn't build):
> proto 17: 72437 pkt/s
> proto 17: 72510 pkt/s
> proto 17: 72479 pkt/s
> proto 17: 72499 pkt/s
> proto 17: 72497 pkt/s
> proto 17: 72427 pkt/s
>
> With the change I pasted above:
> proto 17: 71891 pkt/s
> proto 17: 71810 pkt/s
> proto 17: 71850 pkt/s
> proto 17: 71826 pkt/s
> proto 17: 71798 pkt/s
> proto 17: 71786 pkt/s
> proto 17: 71814 pkt/s
> proto 17: 71814 pkt/s
> proto 17: 72010 pkt/s
>
> So basically, not only are we better off just zero-initializing the complete
> skb->cb instead of looking up the tagger's skb_cb_size, but zero-initializing the
> skb->cb isn't even all that bad. Yangbo's change is an overall win anyway, all
> things considered. So just change the memset as Richard suggested, make sure
> all patches compile, and we should be good to go.
Ah... I had thought 48bytes memset was acceptable for now by you.
I actually didn't consider the performance affected by this. I just did this because I believed the direction was right:)
That's really good testing. Thank you very much, Vladimir.
With the data, we can feel free to use the changes.
> -----Original Message-----
> From: Richard Cochran <[email protected]>
> Sent: 2021??4??26?? 21:39
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; Vladimir Oltean <[email protected]>;
> David S . Miller <[email protected]>; Jakub Kicinski <[email protected]>;
> Jonathan Corbet <[email protected]>; Kurt Kanzenbach <[email protected]>;
> Andrew Lunn <[email protected]>; Vivien Didelot <[email protected]>;
> Florian Fainelli <[email protected]>; Claudiu Manoil
> <[email protected]>; Alexandre Belloni
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver
>
> On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> > @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff
> *skb, struct net_device *dev)
> >
> > dev_sw_netstats_tx_add(dev, 1, skb->len);
> >
> > - DSA_SKB_CB(skb)->clone = NULL;
> > + memset(skb->cb, 0, 48);
>
> Replace hard coded 48 with sizeof() please.
Fixed in v3.
Thank you!
>
> Thanks,
> Richard
On Mon, Apr 26, 2021 at 09:39:44PM +0300, Vladimir Oltean wrote:
> On Mon, Apr 26, 2021 at 06:38:46AM -0700, Richard Cochran wrote:
> > On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> > > @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
> > >
> > > dev_sw_netstats_tx_add(dev, 1, skb->len);
> > >
> > > - DSA_SKB_CB(skb)->clone = NULL;
> > > + memset(skb->cb, 0, 48);
> >
> > Replace hard coded 48 with sizeof() please.
>
> You mean just a trivial change like this, right?
>
> memset(skb->cb, 0, sizeof(skb->cb));
Yes.
Thanks,
Richard
On Mon, Apr 26, 2021 at 17:37, Yangbo Lu <[email protected]> wrote:
> Free skb->cb usage in core driver and let device drivers decide to
> use or not. The reason having a DSA_SKB_CB(skb)->clone was because
> dsa_skb_tx_timestamp() which may set the clone pointer was called
> before p->xmit() which would use the clone if any, and the device
> driver has no way to initialize the clone pointer.
>
> Although for now putting memset(skb->cb, 0, 48) at beginning of
> dsa_slave_xmit() by this patch is not very good, there is still way
> to improve this. Otherwise, some other new features, like one-step
Could you please expand on this improvement?
This memset makes it impossible to carry control buffer information from
driver callbacks that run before .ndo_start_xmit, for example
.ndo_select_queue, to a tagger's .xmit.
It seems to me that if the drivers are to handle the CB internally from
now on, that should go for both setting and clearing of the required
fields.
> timestamp which needs a flag of skb marked in dsa_skb_tx_timestamp(),
> and handles as one-step timestamp in p->xmit() will face same
> situation.
>
> Signed-off-by: Yangbo Lu <[email protected]>
> ---
> Changes for v2:
> - Added this patch.
> ---
> drivers/net/dsa/ocelot/felix.c | 1 +
> drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
> drivers/net/dsa/sja1105/sja1105_ptp.c | 4 +++-
> drivers/net/ethernet/mscc/ocelot.c | 6 +++---
> drivers/net/ethernet/mscc/ocelot_net.c | 2 +-
> include/linux/dsa/sja1105.h | 3 ++-
> include/net/dsa.h | 14 --------------
> include/soc/mscc/ocelot.h | 8 ++++++++
> net/dsa/slave.c | 3 +--
> net/dsa/tag_ocelot.c | 8 ++++----
> net/dsa/tag_ocelot_8021q.c | 8 ++++----
> 11 files changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index d679f023dc00..8980d56ee793 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -1403,6 +1403,7 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
>
> if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> + OCELOT_SKB_CB(skb)->clone = clone;
> return true;
> }
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index d9c198ca0197..405024b637d6 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -3137,7 +3137,7 @@ static void sja1105_port_deferred_xmit(struct kthread_work *work)
> struct sk_buff *skb;
>
> while ((skb = skb_dequeue(&sp->xmit_queue)) != NULL) {
> - struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
> + struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone;
>
> mutex_lock(&priv->mgmt_lock);
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
> index 72d052de82d8..0832368aaa96 100644
> --- a/drivers/net/dsa/sja1105/sja1105_ptp.c
> +++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
> @@ -432,7 +432,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
> }
>
> /* Called from dsa_skb_tx_timestamp. This callback is just to make DSA clone
> - * the skb and have it available in DSA_SKB_CB in the .port_deferred_xmit
> + * the skb and have it available in SJA1105_SKB_CB in the .port_deferred_xmit
> * callback, where we will timestamp it synchronously.
> */
> bool sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> @@ -443,6 +443,8 @@ bool sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> if (!sp->hwts_tx_en)
> return false;
>
> + SJA1105_SKB_CB(skb)->clone = clone;
> +
> return true;
> }
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 8d06ffaf318a..7da2dd1632b1 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -538,8 +538,8 @@ void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
> spin_lock(&ocelot_port->ts_id_lock);
>
> skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
> - /* Store timestamp ID in cb[0] of sk_buff */
> - clone->cb[0] = ocelot_port->ts_id;
> + /* Store timestamp ID in OCELOT_SKB_CB(clone)->ts_id */
> + OCELOT_SKB_CB(clone)->ts_id = ocelot_port->ts_id;
> ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;
> skb_queue_tail(&ocelot_port->tx_skbs, clone);
>
> @@ -604,7 +604,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
> spin_lock_irqsave(&port->tx_skbs.lock, flags);
>
> skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> - if (skb->cb[0] != id)
> + if (OCELOT_SKB_CB(skb)->ts_id != id)
> continue;
> __skb_unlink(skb, &port->tx_skbs);
> skb_match = skb;
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 36f32a4d9b0f..789a5fba146c 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -520,7 +520,7 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
>
> ocelot_port_add_txtstamp_skb(ocelot, port, clone);
>
> - rew_op |= clone->cb[0] << 3;
> + rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
> }
> }
>
> diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
> index dd93735ae228..1eb84562b311 100644
> --- a/include/linux/dsa/sja1105.h
> +++ b/include/linux/dsa/sja1105.h
> @@ -47,11 +47,12 @@ struct sja1105_tagger_data {
> };
>
> struct sja1105_skb_cb {
> + struct sk_buff *clone;
> u32 meta_tstamp;
> };
>
> #define SJA1105_SKB_CB(skb) \
> - ((struct sja1105_skb_cb *)DSA_SKB_CB_PRIV(skb))
> + ((struct sja1105_skb_cb *)((skb)->cb))
>
> struct sja1105_port {
> u16 subvlan_map[DSA_8021Q_N_SUBVLAN];
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 905066055b08..5685e60cb082 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -117,20 +117,6 @@ struct dsa_netdevice_ops {
> #define MODULE_ALIAS_DSA_TAG_DRIVER(__proto) \
> MODULE_ALIAS(DSA_TAG_DRIVER_ALIAS __stringify(__proto##_VALUE))
>
> -struct dsa_skb_cb {
> - struct sk_buff *clone;
> -};
> -
> -struct __dsa_skb_cb {
> - struct dsa_skb_cb cb;
> - u8 priv[48 - sizeof(struct dsa_skb_cb)];
> -};
> -
> -#define DSA_SKB_CB(skb) ((struct dsa_skb_cb *)((skb)->cb))
> -
> -#define DSA_SKB_CB_PRIV(skb) \
> - ((void *)(skb)->cb + offsetof(struct __dsa_skb_cb, priv))
> -
> struct dsa_switch_tree {
> struct list_head list;
>
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 68cdc7ceaf4d..f075aaf70eee 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -689,6 +689,14 @@ struct ocelot_policer {
> u32 burst; /* bytes */
> };
>
> +struct ocelot_skb_cb {
> + struct sk_buff *clone;
> + u8 ts_id;
> +};
> +
> +#define OCELOT_SKB_CB(skb) \
> + ((struct ocelot_skb_cb *)((skb)->cb))
> +
> #define ocelot_read_ix(ocelot, reg, gi, ri) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
> #define ocelot_read_gix(ocelot, reg, gi) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi))
> #define ocelot_read_rix(ocelot, reg, ri) __ocelot_read_ix(ocelot, reg, reg##_RSZ * (ri))
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index acaa52e60d7f..2211894c2381 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -568,7 +568,6 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
> return;
>
> if (ds->ops->port_txtstamp(ds, p->dp->index, clone)) {
> - DSA_SKB_CB(skb)->clone = clone;
> return;
> }
>
> @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>
> dev_sw_netstats_tx_add(dev, 1, skb->len);
>
> - DSA_SKB_CB(skb)->clone = NULL;
> + memset(skb->cb, 0, 48);
>
> /* Handle tx timestamp if any */
> dsa_skb_tx_timestamp(p, skb);
> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
> index f9df9cac81c5..1100a16f1032 100644
> --- a/net/dsa/tag_ocelot.c
> +++ b/net/dsa/tag_ocelot.c
> @@ -15,11 +15,11 @@ static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
> ocelot_port = ocelot->ports[dp->index];
> rew_op = ocelot_port->ptp_cmd;
>
> - /* Retrieve timestamp ID populated inside skb->cb[0] of the
> - * clone by ocelot_port_add_txtstamp_skb
> + /* Retrieve timestamp ID populated inside OCELOT_SKB_CB(clone)->ts_id
> + * by ocelot_port_add_txtstamp_skb
> */
> if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
> - rew_op |= clone->cb[0] << 3;
> + rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
>
> ocelot_ifh_set_rew_op(injection, rew_op);
> }
> @@ -28,7 +28,7 @@ static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
> __be32 ifh_prefix, void **ifh)
> {
> struct dsa_port *dp = dsa_slave_to_port(netdev);
> - struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
> + struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
> struct dsa_switch *ds = dp->ds;
> void *injection;
> __be32 *prefix;
> diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
> index 5f3e8e124a82..a001a7e3f575 100644
> --- a/net/dsa/tag_ocelot_8021q.c
> +++ b/net/dsa/tag_ocelot_8021q.c
> @@ -28,11 +28,11 @@ static struct sk_buff *ocelot_xmit_ptp(struct dsa_port *dp,
> ocelot_port = ocelot->ports[port];
> rew_op = ocelot_port->ptp_cmd;
>
> - /* Retrieve timestamp ID populated inside skb->cb[0] of the
> - * clone by ocelot_port_add_txtstamp_skb
> + /* Retrieve timestamp ID populated inside OCELOT_SKB_CB(clone)->ts_id
> + * by ocelot_port_add_txtstamp_skb
> */
> if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
> - rew_op |= clone->cb[0] << 3;
> + rew_op |= OCELOT_SKB_CB(clone)->ts_id << 3;
>
> ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
>
> @@ -46,7 +46,7 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
> u16 tx_vid = dsa_8021q_tx_vid(dp->ds, dp->index);
> u16 queue_mapping = skb_get_queue_mapping(skb);
> u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
> - struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
> + struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone;
>
> /* TX timestamping was requested, so inject through MMIO */
> if (clone)
> --
> 2.25.1
On Fri, May 07, 2021 at 11:26:32AM +0000, Y.b. Lu wrote:
> > From: Tobias Waldekranz <[email protected]>
> > On Mon, Apr 26, 2021 at 17:37, Yangbo Lu <[email protected]> wrote:
> > > Free skb->cb usage in core driver and let device drivers decide to use
> > > or not. The reason having a DSA_SKB_CB(skb)->clone was because
> > > dsa_skb_tx_timestamp() which may set the clone pointer was called
> > > before p->xmit() which would use the clone if any, and the device
> > > driver has no way to initialize the clone pointer.
> > >
> > > Although for now putting memset(skb->cb, 0, 48) at beginning of
> > > dsa_slave_xmit() by this patch is not very good, there is still way to
> > > improve this. Otherwise, some other new features, like one-step
> >
> > Could you please expand on this improvement?
> >
> > This memset makes it impossible to carry control buffer information from
> > driver callbacks that run before .ndo_start_xmit, for
> > example .ndo_select_queue, to a tagger's .xmit.
> >
> > It seems to me that if the drivers are to handle the CB internally from now on,
> > that should go for both setting and clearing of the required fields.
>
> For the timestamping case, dsa_skb_tx_timestamp may not touch
> .port_txtstamp callback, so we had to put skb->cb initialization at
> beginning of dsa_slave_xmit.
> To avoid breaking future skb->cb usage you mentioned, do you think we
> can back to Vladimir's idea initializing only field required, or even
> just add a callback for cb initialization for timestamping?
I would like to avoid overengineering, which a callback for skb->cb
initialization would introduce, given the needs we have now.
FWIW, we may not even be able to touch skb->cb in .ndo_select_queue for
Tobias's use case, that discussion is here:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> -----Original Message-----
> From: Tobias Waldekranz <[email protected]>
> Sent: 2021??4??29?? 4:30
> To: Y.b. Lu <[email protected]>; [email protected]
> Cc: Y.b. Lu <[email protected]>; Richard Cochran
> <[email protected]>; Vladimir Oltean <[email protected]>;
> David S . Miller <[email protected]>; Jakub Kicinski <[email protected]>;
> Jonathan Corbet <[email protected]>; Kurt Kanzenbach <[email protected]>;
> Andrew Lunn <[email protected]>; Vivien Didelot <[email protected]>;
> Florian Fainelli <[email protected]>; Claudiu Manoil
> <[email protected]>; Alexandre Belloni
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver
>
> On Mon, Apr 26, 2021 at 17:37, Yangbo Lu <[email protected]> wrote:
> > Free skb->cb usage in core driver and let device drivers decide to use
> > or not. The reason having a DSA_SKB_CB(skb)->clone was because
> > dsa_skb_tx_timestamp() which may set the clone pointer was called
> > before p->xmit() which would use the clone if any, and the device
> > driver has no way to initialize the clone pointer.
> >
> > Although for now putting memset(skb->cb, 0, 48) at beginning of
> > dsa_slave_xmit() by this patch is not very good, there is still way to
> > improve this. Otherwise, some other new features, like one-step
>
> Could you please expand on this improvement?
>
> This memset makes it impossible to carry control buffer information from
> driver callbacks that run before .ndo_start_xmit, for
> example .ndo_select_queue, to a tagger's .xmit.
>
> It seems to me that if the drivers are to handle the CB internally from now on,
> that should go for both setting and clearing of the required fields.
>
For the timestamping case, dsa_skb_tx_timestamp may not touch .port_txtstamp callback, so we had to put skb->cb initialization at beginning of dsa_slave_xmit.
To avoid breaking future skb->cb usage you mentioned, do you think we can back to Vladimir's idea initializing only field required, or even just add a callback for cb initialization for timestamping?
Thanks.
> > timestamp which needs a flag of skb marked in dsa_skb_tx_timestamp(),
> > and handles as one-step timestamp in p->xmit() will face same
> > situation.
> >
> > Signed-off-by: Yangbo Lu <[email protected]>