Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code
On 12/08/2018 12:34 PM, Andrew Lunn wrote:
> On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote:
>> From: Tristram Ha
>>
>> Factor out common code from the tag_ksz , so that the code can be used
>> with other KSZ family switches which use differenly sized tags.
>
> I prefer this implementation over what Tristram recently submitted. It
> is also what we suggested a while back. However, since then we have
> had Spectra/meltdown, and we now know a function call through a
> pointer is expensive. This is the hot path, every frame comes through
> here, so it is worth taking the time to optimize this. Could you try
> to remove the ksz_tag_ops structure. xmit looks simple, since it is a
> tail call, so you can do that in ksz9477_xmit. Receive looks a bit
> more complex.
They're both simple in the end, I'll send V2 once I test it a bit.
> I also think for the moment we need it ignore PTP until we have the
> big picture sorted out. If need be, the code can be refactored yet
> again. But i don't want PTP holding up getting more switches
> supported.
>
>> Signed-off-by: Tristram Ha
>> Signed-off-by: Marek Vasut
>> Cc: Vivien Didelot
>> Cc: Woojung Huh
>> Cc: David S. Miller
>> ---
>> net/dsa/tag_ksz.c | 125 +-
>> 1 file changed, 90 insertions(+), 35 deletions(-)
>>
>> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
>> index 036bc62198f28..d94bad1ab7e53 100644
>> --- a/net/dsa/tag_ksz.c
>> +++ b/net/dsa/tag_ksz.c
>> @@ -14,34 +14,30 @@
>> #include
>> #include "dsa_priv.h"
>>
>> -/* For Ingress (Host -> KSZ), 2 bytes are added before FCS.
>> - *
>> ---
>> - *
>> DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
>> - *
>> ---
>> - * tag0 : Prioritization (not used now)
>> - * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
>> - *
>> - * For Egress (KSZ -> Host), 1 byte is added before FCS.
>> - *
>> ---
>> - * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|FCS(4bytes)
>> - *
>> ---
>> - * tag0 : zero-based value represents port
>> - *(eg, 0x00=port1, 0x02=port3, 0x06=port7)
>> - */
>> +struct ksz_tag_ops {
>> +void(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
>> +void(*set_tag)(struct sk_buff *skb, struct net_device *dev);
>> +};
>>
>> -#define KSZ_INGRESS_TAG_LEN 2
>> -#define KSZ_EGRESS_TAG_LEN 1
>> +/* Typically only one byte is used for tail tag. */
>> +#define KSZ_EGRESS_TAG_LEN 1
>>
>> -static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
>> +/* Frames with following addresse may need to be sent even when the port is
>> + * closed.
>> + */
>> +static const u8 special_mult_addr[] = {
>> +0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
>> +};
>> +
>
> This is new functionality, not part of the refactoring the
> code. Please add that as a new patch. Also, you can probably make use
> of is_link_local_ether_addr().
Sounds good, will do in V2.
--
Best regards,
Marek Vasut
Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code
> So this ksz_tag_ops is still not acceptable? As I understand the kernel is > using this mechanism all over the places. Hi Tristram It is used all other the place, but generally, not in the hot path, just the control plain. > What is left is a direct copying of the transmit and receive functions for > each > new switch tail tag format. As i said, one of these looks easy to achieve, the other i did not look at too closely, but needs more work. Andrew
Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code
On 12/10/2018 09:32 PM, tristram...@microchip.com wrote: >> On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote: >>> From: Tristram Ha >>> >>> Factor out common code from the tag_ksz , so that the code can be used >>> with other KSZ family switches which use differenly sized tags. >> >> I prefer this implementation over what Tristram recently submitted. It >> is also what we suggested a while back. However, since then we have >> had Spectra/meltdown, and we now know a function call through a >> pointer is expensive. This is the hot path, every frame comes through >> here, so it is worth taking the time to optimize this. Could you try >> to remove the ksz_tag_ops structure. xmit looks simple, since it is a >> tail call, so you can do that in ksz9477_xmit. Receive looks a bit >> more complex. >> >> I also think for the moment we need it ignore PTP until we have the >> big picture sorted out. If need be, the code can be refactored yet >> again. But i don't want PTP holding up getting more switches >> supported. > > Yes, as you may already know, what Marek submitted was my previous > attempt to support different switches in tag_ksz.c. I adjusted the code quite a bit, please look again. > So this ksz_tag_ops is still not acceptable? As I understand the kernel is > using this mechanism all over the places. > > What is left is a direct copying of the transmit and receive functions for > each > new switch tail tag format. Maybe a macro would work ? Or some code runtime-patching ? -- Best regards, Marek Vasut
RE: [PATCH 3/5] net: dsa: ksz: Factor out common tag code
> On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote: > > From: Tristram Ha > > > > Factor out common code from the tag_ksz , so that the code can be used > > with other KSZ family switches which use differenly sized tags. > > I prefer this implementation over what Tristram recently submitted. It > is also what we suggested a while back. However, since then we have > had Spectra/meltdown, and we now know a function call through a > pointer is expensive. This is the hot path, every frame comes through > here, so it is worth taking the time to optimize this. Could you try > to remove the ksz_tag_ops structure. xmit looks simple, since it is a > tail call, so you can do that in ksz9477_xmit. Receive looks a bit > more complex. > > I also think for the moment we need it ignore PTP until we have the > big picture sorted out. If need be, the code can be refactored yet > again. But i don't want PTP holding up getting more switches > supported. Yes, as you may already know, what Marek submitted was my previous attempt to support different switches in tag_ksz.c. So this ksz_tag_ops is still not acceptable? As I understand the kernel is using this mechanism all over the places. What is left is a direct copying of the transmit and receive functions for each new switch tail tag format.
Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code
On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote:
> From: Tristram Ha
>
> Factor out common code from the tag_ksz , so that the code can be used
> with other KSZ family switches which use differenly sized tags.
I prefer this implementation over what Tristram recently submitted. It
is also what we suggested a while back. However, since then we have
had Spectra/meltdown, and we now know a function call through a
pointer is expensive. This is the hot path, every frame comes through
here, so it is worth taking the time to optimize this. Could you try
to remove the ksz_tag_ops structure. xmit looks simple, since it is a
tail call, so you can do that in ksz9477_xmit. Receive looks a bit
more complex.
I also think for the moment we need it ignore PTP until we have the
big picture sorted out. If need be, the code can be refactored yet
again. But i don't want PTP holding up getting more switches
supported.
> Signed-off-by: Tristram Ha
> Signed-off-by: Marek Vasut
> Cc: Vivien Didelot
> Cc: Woojung Huh
> Cc: David S. Miller
> ---
> net/dsa/tag_ksz.c | 125 +-
> 1 file changed, 90 insertions(+), 35 deletions(-)
>
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 036bc62198f28..d94bad1ab7e53 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -14,34 +14,30 @@
> #include
> #include "dsa_priv.h"
>
> -/* For Ingress (Host -> KSZ), 2 bytes are added before FCS.
> - *
> ---
> - *
> DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> - *
> ---
> - * tag0 : Prioritization (not used now)
> - * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
> - *
> - * For Egress (KSZ -> Host), 1 byte is added before FCS.
> - *
> ---
> - * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|FCS(4bytes)
> - *
> ---
> - * tag0 : zero-based value represents port
> - * (eg, 0x00=port1, 0x02=port3, 0x06=port7)
> - */
> +struct ksz_tag_ops {
> + void(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
> + void(*set_tag)(struct sk_buff *skb, struct net_device *dev);
> +};
>
> -#define KSZ_INGRESS_TAG_LEN 2
> -#define KSZ_EGRESS_TAG_LEN 1
> +/* Typically only one byte is used for tail tag. */
> +#define KSZ_EGRESS_TAG_LEN 1
>
> -static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
> +/* Frames with following addresse may need to be sent even when the port is
> + * closed.
> + */
> +static const u8 special_mult_addr[] = {
> + 0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
> +};
> +
This is new functionality, not part of the refactoring the
code. Please add that as a new patch. Also, you can probably make use
of is_link_local_ether_addr().
Andrew
[PATCH 3/5] net: dsa: ksz: Factor out common tag code
From: Tristram Ha
Factor out common code from the tag_ksz , so that the code can be used
with other KSZ family switches which use differenly sized tags.
Signed-off-by: Tristram Ha
Signed-off-by: Marek Vasut
Cc: Vivien Didelot
Cc: Woojung Huh
Cc: David S. Miller
---
net/dsa/tag_ksz.c | 125 +-
1 file changed, 90 insertions(+), 35 deletions(-)
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 036bc62198f28..d94bad1ab7e53 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -14,34 +14,30 @@
#include
#include "dsa_priv.h"
-/* For Ingress (Host -> KSZ), 2 bytes are added before FCS.
- * ---
- * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
- * ---
- * tag0 : Prioritization (not used now)
- * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
- *
- * For Egress (KSZ -> Host), 1 byte is added before FCS.
- * ---
- * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|FCS(4bytes)
- * ---
- * tag0 : zero-based value represents port
- * (eg, 0x00=port1, 0x02=port3, 0x06=port7)
- */
+struct ksz_tag_ops {
+ void(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
+ void(*set_tag)(struct sk_buff *skb, struct net_device *dev);
+};
-#defineKSZ_INGRESS_TAG_LEN 2
-#defineKSZ_EGRESS_TAG_LEN 1
+/* Typically only one byte is used for tail tag. */
+#define KSZ_EGRESS_TAG_LEN 1
-static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
+/* Frames with following addresse may need to be sent even when the port is
+ * closed.
+ */
+static const u8 special_mult_addr[] = {
+ 0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
+};
+
+static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev,
+ struct ksz_tag_ops *ops, int len)
{
- struct dsa_port *dp = dsa_slave_to_port(dev);
struct sk_buff *nskb;
int padlen;
- u8 *tag;
padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb->len;
- if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) {
+ if (skb_tailroom(skb) >= padlen + len) {
/* Let dsa_slave_xmit() free skb */
if (__skb_put_padto(skb, skb->len + padlen, false))
return NULL;
@@ -49,7 +45,7 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct
net_device *dev)
nskb = skb;
} else {
nskb = alloc_skb(NET_IP_ALIGN + skb->len +
-padlen + KSZ_INGRESS_TAG_LEN, GFP_ATOMIC);
+padlen + len, GFP_ATOMIC);
if (!nskb)
return NULL;
skb_reserve(nskb, NET_IP_ALIGN);
@@ -70,34 +66,93 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct
net_device *dev)
consume_skb(skb);
}
- tag = skb_put(nskb, KSZ_INGRESS_TAG_LEN);
- tag[0] = 0;
- tag[1] = 1 << dp->index; /* destination port */
+ ops->set_tag(nskb, dev);
return nskb;
}
static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt)
+ struct packet_type *pt, struct ksz_tag_ops *ops)
{
- u8 *tag;
- int source_port;
+ int sp, len;
+ u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
- tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+ ops->get_tag(tag, &sp, &len);
- source_port = tag[0] & 7;
-
- skb->dev = dsa_master_find_slave(dev, 0, source_port);
+ skb->dev = dsa_master_find_slave(dev, 0, sp);
if (!skb->dev)
return NULL;
- pskb_trim_rcsum(skb, skb->len - KSZ_EGRESS_TAG_LEN);
+ pskb_trim_rcsum(skb, skb->len - len);
return skb;
}
+/*
+ * For Ingress (Host -> KSZ9477), 2 bytes are added before FCS.
+ * ---
+ * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
+ * ---
+ * tag0 : Prioritization (not used now)
+ * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
+ *
+ * For Egress (KSZ9477 -> Host), 1 byte is added before FCS.
+ * ---
+ * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|FCS(4bytes)
+ * ---
+ * tag0 : zero-based value represents port
+ * (eg, 0x00=port1, 0x
