On Tue, Jan 24, 2023 at 05:40:55PM +0300, Vitaliy Makkoveev wrote: > On Tue, Jan 24, 2023 at 03:14:36PM +0100, Jan Klemkow wrote: > > On Tue, Jan 24, 2023 at 09:32:53PM +1000, David Gwynne wrote: > > > On Mon, Jan 23, 2023 at 09:25:34AM +0100, Jan Klemkow wrote: > > > > On Wed, Jan 18, 2023 at 03:49:25PM -0700, Theo de Raadt wrote: > > > > > Jan Klemkow <j.klem...@wemelug.de> wrote: > > > > > > On Wed, Jan 18, 2023 at 10:50:25AM +0300, Vitaliy Makkoveev wrote: > > > > > > > On Tue, Jan 17, 2023 at 11:09:17PM +0100, Jan Klemkow wrote: > > > > > > > > we have several drivers which have to parse the content of > > > > > > > > mbufs. This > > > > > > > > diff suggest a central parsing function for this. Thus, we can > > > > > > > > reduce > > > > > > > > redundant code. > > > > > > > > > > > > > > > > I just start with ix(4) and ixl(4) because it was easy to test > > > > > > > > for me. > > > > > > > > But, this could also improve em(4), igc(4), ale(4) and oce(4). > > > > > > > > > > > > > > > > I'm not sure about the name, the api nor the place of this > > > > > > > > code. So, if > > > > > > > > someone has a better idea: i'm open to anything. > > > > > > > > > > > > > > I like code this deduplication. > > > > > > > > > > > > > > This newly introduced function doesn't touch ifnet but only > > > > > > > extracts > > > > > > > protocol headers from mbuf(9). I guess mbuf_extract_headers() or > > > > > > > something like is much better for name with the ern/uipc_mbuf2.c > > > > > > > as > > > > > > > place. > > > > > > > > > > > > Good Point. Updates diff below. > > > > > > > > > > I agree, "extract" is a better name. dlg, do you have a comment? > > > > > > > > Whats you opinion about this diff? > > > > > > it makes ix and ixl prettier, so that's a good enough reason to do > > > it. it should go in net/if_ethersubr.c as ether_extract_headers() > > > though. > > > > > > could you try using a struct to carry the header pointers around and see > > > what that looks like? > > > > > > struct ether_extracted { > > > struct ether_header *eh; > > > struct ip *ip4; > > > struct ip6_hdr *ip6; > > > struct tcphdr *tcp; > > > struct udphdr *udp; > > > }; > > > > > > void ether_extract_headers(struct mbuf *, struct ether_extracted *); > > > > > > you can add a depth or flags argument if you want to be able to > > > tell it to return before looking for the tcp/udp headers if you > > > want. > > Looks better then m_extract_headers(). Since ext->eh is always assigned > to non NULL value below, the "ext->eh = NULL;" is not necessary. Also > I'm not sure, but is memset() more reliable for `ext' zeroing? Anyway, > feel free to commit without memset().
OK? Thanks, Jan Index: dev/pci/if_ix.c =================================================================== RCS file: /cvs/src/sys/dev/pci/if_ix.c,v retrieving revision 1.189 diff -u -p -r1.189 if_ix.c --- dev/pci/if_ix.c 2 Sep 2022 14:08:09 -0000 1.189 +++ dev/pci/if_ix.c 24 Jan 2023 13:34:17 -0000 @@ -2477,25 +2477,16 @@ static inline int ixgbe_csum_offload(struct mbuf *mp, uint32_t *vlan_macip_lens, uint32_t *type_tucmd_mlhl, uint32_t *olinfo_status) { - struct ether_header *eh = mtod(mp, struct ether_header *); - struct mbuf *m; - int hoff; + struct ether_extracted ext; int offload = 0; uint32_t iphlen; - uint8_t ipproto; - *vlan_macip_lens |= (sizeof(*eh) << IXGBE_ADVTXD_MACLEN_SHIFT); + ether_extract_headers(mp, &ext); - switch (ntohs(eh->ether_type)) { - case ETHERTYPE_IP: { - struct ip *ip; + *vlan_macip_lens |= (sizeof(*ext.eh) << IXGBE_ADVTXD_MACLEN_SHIFT); - m = m_getptr(mp, sizeof(*eh), &hoff); - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip)); - ip = (struct ip *)(mtod(m, caddr_t) + hoff); - - iphlen = ip->ip_hl << 2; - ipproto = ip->ip_p; + if (ext.ip4) { + iphlen = ext.ip4->ip_hl << 2; if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) { *olinfo_status |= IXGBE_TXD_POPTS_IXSM << 8; @@ -2503,46 +2494,30 @@ ixgbe_csum_offload(struct mbuf *mp, uint } *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV4; - break; - } - #ifdef INET6 - case ETHERTYPE_IPV6: { - struct ip6_hdr *ip6; - - m = m_getptr(mp, sizeof(*eh), &hoff); - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip6)); - ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff); - - iphlen = sizeof(*ip6); - ipproto = ip6->ip6_nxt; + } else if (ext.ip6) { + iphlen = sizeof(*ext.ip6); *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6; - break; - } #endif - - default: + } else { return offload; } *vlan_macip_lens |= iphlen; - switch (ipproto) { - case IPPROTO_TCP: + if (ext.tcp) { *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_TCP; if (ISSET(mp->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) { *olinfo_status |= IXGBE_TXD_POPTS_TXSM << 8; offload = 1; } - break; - case IPPROTO_UDP: + } else if (ext.udp) { *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_UDP; if (ISSET(mp->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) { *olinfo_status |= IXGBE_TXD_POPTS_TXSM << 8; offload = 1; } - break; } return offload; Index: dev/pci/if_ixl.c =================================================================== RCS file: /cvs/src/sys/dev/pci/if_ixl.c,v retrieving revision 1.84 diff -u -p -r1.84 if_ixl.c --- dev/pci/if_ixl.c 5 Aug 2022 13:57:16 -0000 1.84 +++ dev/pci/if_ixl.c 24 Jan 2023 13:31:02 -0000 @@ -2784,10 +2784,8 @@ ixl_load_mbuf(bus_dma_tag_t dmat, bus_dm static uint64_t ixl_tx_setup_offload(struct mbuf *m0) { - struct mbuf *m; - int hoff; + struct ether_extracted ext; uint64_t hlen; - uint8_t ipproto; uint64_t offload = 0; if (ISSET(m0->m_flags, M_VLANTAG)) { @@ -2800,39 +2798,21 @@ ixl_tx_setup_offload(struct mbuf *m0) M_IPV4_CSUM_OUT|M_TCP_CSUM_OUT|M_UDP_CSUM_OUT)) return (offload); - switch (ntohs(mtod(m0, struct ether_header *)->ether_type)) { - case ETHERTYPE_IP: { - struct ip *ip; - - m = m_getptr(m0, ETHER_HDR_LEN, &hoff); - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip)); - ip = (struct ip *)(mtod(m, caddr_t) + hoff); + ether_extract_headers(m0, &ext); + if (ext.ip4) { offload |= ISSET(m0->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT) ? IXL_TX_DESC_CMD_IIPT_IPV4_CSUM : IXL_TX_DESC_CMD_IIPT_IPV4; - hlen = ip->ip_hl << 2; - ipproto = ip->ip_p; - break; - } - + hlen = ext.ip4->ip_hl << 2; #ifdef INET6 - case ETHERTYPE_IPV6: { - struct ip6_hdr *ip6; - - m = m_getptr(m0, ETHER_HDR_LEN, &hoff); - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip6)); - ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff); - + } else if (ext.ip6) { offload |= IXL_TX_DESC_CMD_IIPT_IPV6; - hlen = sizeof(*ip6); - ipproto = ip6->ip6_nxt; - break; - } + hlen = sizeof(*ext.ip6); #endif - default: + } else { panic("CSUM_OUT set for non-IP packet"); /* NOTREACHED */ } @@ -2840,30 +2820,12 @@ ixl_tx_setup_offload(struct mbuf *m0) offload |= (ETHER_HDR_LEN >> 1) << IXL_TX_DESC_MACLEN_SHIFT; offload |= (hlen >> 2) << IXL_TX_DESC_IPLEN_SHIFT; - switch (ipproto) { - case IPPROTO_TCP: { - struct tcphdr *th; - - if (!ISSET(m0->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) - break; - - m = m_getptr(m, hoff + hlen, &hoff); - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*th)); - th = (struct tcphdr *)(mtod(m, caddr_t) + hoff); - + if (ext.tcp && ISSET(m0->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) { offload |= IXL_TX_DESC_CMD_L4T_EOFT_TCP; - offload |= (uint64_t)th->th_off << IXL_TX_DESC_L4LEN_SHIFT; - break; - } - - case IPPROTO_UDP: - if (!ISSET(m0->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) - break; - + offload |= (uint64_t)ext.tcp->th_off << IXL_TX_DESC_L4LEN_SHIFT; + } else if (ext.udp && ISSET(m0->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) { offload |= IXL_TX_DESC_CMD_L4T_EOFT_UDP; - offload |= (sizeof(struct udphdr) >> 2) << - IXL_TX_DESC_L4LEN_SHIFT; - break; + offload |= (sizeof(*ext.udp) >> 2) << IXL_TX_DESC_L4LEN_SHIFT; } return (offload); Index: net/if_ethersubr.c =================================================================== RCS file: /cvs/src/sys/net/if_ethersubr.c,v retrieving revision 1.284 diff -u -p -r1.284 if_ethersubr.c --- net/if_ethersubr.c 29 Jun 2022 09:08:07 -0000 1.284 +++ net/if_ethersubr.c 24 Jan 2023 15:19:21 -0000 @@ -98,6 +98,10 @@ didn't get a copy, you may request one f #include <netinet/in.h> #include <netinet/if_ether.h> #include <netinet/ip_ipsp.h> +#include <netinet/ip.h> +#include <netinet/ip6.h> +#include <netinet/tcp.h> +#include <netinet/udp.h> #if NBPFILTER > 0 #include <net/bpf.h> @@ -1033,4 +1037,57 @@ ether_e64_to_addr(struct ether_addr *ea, ea->ether_addr_octet[--i] = e64; e64 >>= 8; } while (i > 0); +} + +/* Parse different TCP/IP protocol headers for a quick view inside an mbuf. */ +void +ether_extract_headers(struct mbuf *mp, struct ether_extracted *ext) +{ + struct mbuf *m; + uint64_t hlen; + int hoff; + uint8_t ipproto; + + /* Return NULL if header was not recognized. */ + memset(ext, 0, sizeof *ext); + + ext->eh = mtod(mp, struct ether_header *); + switch (ntohs(ext->eh->ether_type)) { + case ETHERTYPE_IP: + m = m_getptr(mp, sizeof(*ext->eh), &hoff); + KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ext->ip4)); + ext->ip4 = (struct ip *)(mtod(m, caddr_t) + hoff); + + hlen = ext->ip4->ip_hl << 2; + ipproto = ext->ip4->ip_p; + + break; +#ifdef INET6 + case ETHERTYPE_IPV6: + m = m_getptr(mp, sizeof(*ext->eh), &hoff); + KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ext->ip6)); + ext->ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff); + + hlen = sizeof(*ext->ip6); + ipproto = ext->ip6->ip6_nxt; + + break; +#endif + default: + return; + } + + switch (ipproto) { + case IPPROTO_TCP: + m = m_getptr(m, hoff + hlen, &hoff); + KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ext->tcp)); + ext->tcp = (struct tcphdr *)(mtod(m, caddr_t) + hoff); + break; + + case IPPROTO_UDP: + m = m_getptr(m, hoff + hlen, &hoff); + KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ext->udp)); + ext->udp = (struct udphdr *)(mtod(m, caddr_t) + hoff); + break; + } } Index: netinet/if_ether.h =================================================================== RCS file: /cvs/src/sys/netinet/if_ether.h,v retrieving revision 1.84 diff -u -p -r1.84 if_ether.h --- netinet/if_ether.h 27 Dec 2022 20:13:03 -0000 1.84 +++ netinet/if_ether.h 24 Jan 2023 13:57:19 -0000 @@ -297,6 +297,16 @@ const struct ether_brport * uint64_t ether_addr_to_e64(const struct ether_addr *); void ether_e64_to_addr(struct ether_addr *, uint64_t); +struct ether_extracted { + struct ether_header *eh; + struct ip *ip4; + struct ip6_hdr *ip6; + struct tcphdr *tcp; + struct udphdr *udp; +}; + +void ether_extract_headers(struct mbuf *, struct ether_extracted *); + /* * Ethernet multicast address structure. There is one of these for each * multicast address or range of multicast addresses that we are supposed