| Message ID | 20190504011826.30477-5-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Traffic support for SJA1105 DSA driver | expand |
On 5/3/2019 6:18 PM, Vladimir Oltean wrote: > Map a DSA structure over the 48-byte control block that will hold > persistent skb info on transmit and receive. > On receive you cannot quite do that because you don't know if the DSA master network device calls netif_receive_skb() or napi_gro_receive(). The latter arguably may not be able to aggregate flows at all because it does not know how to parse the SKB, but the point remains that skb->cb[] on receive may already be used, up to 48 bytes already. I tried to make use of it for storing the HW extracted Broadcom tag, but it blew the budge on 64-bit hosts: https://www.spinics.net/lists/netdev/msg337777.html Not asking you to change anything here, just to be aware of it. > Also add a DSA_SKB_CB_PRIV() macro which retrieves a pointer to the > space up to 48 bytes that the DSA structure does not use. This space can > be used for drivers to add their own private info. > > One use is for the PTP timestamping code path. When cloning a skb, > annotate the original with a pointer to the clone, which the driver can > then find easily and place the timestamp to. This avoids the need of a > separate queue to hold clones and a way to match an original to a cloned > skb. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Sat, 4 May 2019 at 05:04, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 5/3/2019 6:18 PM, Vladimir Oltean wrote: > > Map a DSA structure over the 48-byte control block that will hold > > persistent skb info on transmit and receive. > > > > On receive you cannot quite do that because you don't know if the DSA > master network device calls netif_receive_skb() or napi_gro_receive(). > The latter arguably may not be able to aggregate flows at all because it > does not know how to parse the SKB, but the point remains that skb->cb[] > on receive may already be used, up to 48 bytes already. I tried to make > use of it for storing the HW extracted Broadcom tag, but it blew the > budge on 64-bit hosts: > > https://www.spinics.net/lists/netdev/msg337777.html > > Not asking you to change anything here, just to be aware of it. > > > Also add a DSA_SKB_CB_PRIV() macro which retrieves a pointer to the > > space up to 48 bytes that the DSA structure does not use. This space can > > be used for drivers to add their own private info. > > > > One use is for the PTP timestamping code path. When cloning a skb, > > annotate the original with a pointer to the clone, which the driver can > > then find easily and place the timestamp to. This avoids the need of a > > separate queue to hold clones and a way to match an original to a cloned > > skb. > > > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > -- > Florian Hi Florian, This is good to know. So I'm understanding that you tried to pass data in the skb->cb between the master netdev and DSA, and GRO got in-between? That's kind of expected in a way. I'm proposing a more minimalistic use even on receive, which should be ok. If you look at 07/09 you'll see I'm setting the frame type from within the eth_type_trans callback (technically still not in the DSA packet_type handler yet, but quite close) and using it in the actual rcv. Then for timestamping I'm communicating between the rcv function and a workqueue that I start from .port_rxtstamp. -Vladimir
diff --git a/include/net/dsa.h b/include/net/dsa.h index c90ceeec7d1f..8f5fcec30b13 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -83,6 +83,34 @@ struct dsa_device_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(skb) ((struct dsa_skb_cb *)((skb)->cb)) + +#define DSA_SKB_CB_COPY(nskb, skb) \ + { *__DSA_SKB_CB(nskb) = *__DSA_SKB_CB(skb); } + +#define DSA_SKB_CB_ZERO(skb) \ + { *__DSA_SKB_CB(skb) = (struct __dsa_skb_cb) {0}; } + +#define DSA_SKB_CB_PRIV(skb) \ + ((void *)(skb)->cb + offsetof(struct __dsa_skb_cb, priv)) + +#define DSA_SKB_CB_CLONE(clone, skb) \ + { \ + DSA_SKB_CB_COPY(clone, skb); \ + DSA_SKB_CB(skb)->clone = clone; \ + } + struct dsa_switch_tree { struct list_head list;
Map a DSA structure over the 48-byte control block that will hold persistent skb info on transmit and receive. Also add a DSA_SKB_CB_PRIV() macro which retrieves a pointer to the space up to 48 bytes that the DSA structure does not use. This space can be used for drivers to add their own private info. One use is for the PTP timestamping code path. When cloning a skb, annotate the original with a pointer to the clone, which the driver can then find easily and place the timestamp to. This avoids the need of a separate queue to hold clones and a way to match an original to a cloned skb. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- include/net/dsa.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)