| Message ID | 20181207181845.21702-3-marex@denx.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/5] net: dsa: ksz: Add MIB counter reading support | expand |
On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote: > From: Tristram Ha <Tristram.Ha@microchip.com> > > 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 <Tristram.Ha@microchip.com> > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > Cc: Woojung Huh <woojung.huh@microchip.com> > Cc: David S. Miller <davem@davemloft.net> > --- > 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 <net/dsa.h> > #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
> On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote: > > From: Tristram Ha <Tristram.Ha@microchip.com> > > > > 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.
On 12/10/2018 09:32 PM, Tristram.Ha@microchip.com wrote: >> On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote: >>> From: Tristram Ha <Tristram.Ha@microchip.com> >>> >>> 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 ?
> 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
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 <Tristram.Ha@microchip.com> >> >> 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 <Tristram.Ha@microchip.com> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >> Cc: Woojung Huh <woojung.huh@microchip.com> >> Cc: David S. Miller <davem@davemloft.net> >> --- >> 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 <net/dsa.h> >> #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.
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 <net/dsa.h> #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 +}; + +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, 0x02=port3, 0x06=port7) + */ + +#define KSZ9477_INGRESS_TAG_LEN 2 +#define KSZ9477_PTP_TAG_LEN 4 +#define KSZ9477_PTP_TAG_INDICATION 0x80 + +#define KSZ9477_TAIL_TAG_OVERRIDE BIT(9) +#define KSZ9477_TAIL_TAG_LOOKUP BIT(10) + +static void ksz9477_get_tag(u8 *tag, unsigned int *port, unsigned int *len) +{ + *port = tag[0] & 7; + *len = KSZ_EGRESS_TAG_LEN; + + /* Extra 4-bytes PTP timestamp */ + if (tag[0] & KSZ9477_PTP_TAG_INDICATION) + *len += KSZ9477_PTP_TAG_LEN; +} + +static void ksz9477_set_tag(struct sk_buff *skb, struct net_device *dev) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + u16 *tag = skb_put(skb, KSZ9477_INGRESS_TAG_LEN); + u8 *addr = skb_mac_header(skb); + + *tag = 1 << dp->index; + if (!memcmp(addr, special_mult_addr, ETH_ALEN)) + *tag |= KSZ9477_TAIL_TAG_OVERRIDE; + + *tag = cpu_to_be16(*tag); +} + +static struct ksz_tag_ops ksz9477_tag_ops = { + .get_tag = ksz9477_get_tag, + .set_tag = ksz9477_set_tag, +}; + +static struct sk_buff *ksz9477_xmit(struct sk_buff *skb, + struct net_device *dev) +{ + return ksz_xmit(skb, dev, &ksz9477_tag_ops, KSZ9477_INGRESS_TAG_LEN); +} + +static struct sk_buff *ksz9477_rcv(struct sk_buff *skb, struct net_device *dev, + struct packet_type *pt) +{ + return ksz_rcv(skb, dev, pt, &ksz9477_tag_ops); +} + const struct dsa_device_ops ksz9477_netdev_ops = { - .xmit = ksz_xmit, - .rcv = ksz_rcv, - .overhead = KSZ_INGRESS_TAG_LEN, + .xmit = ksz9477_xmit, + .rcv = ksz9477_rcv, + .overhead = KSZ9477_INGRESS_TAG_LEN, };