On Thu, Jan 19, 2023 at 10:40:52AM +0100, Jan Klemkow wrote:
> On Thu, Jan 19, 2023 at 12:02:29PM +0300, Vitaliy Makkoveev wrote:
> > On Thu, Jan 19, 2023 at 01:55:57AM +0300, Vitaliy Makkoveev wrote:
> > > > On 19 Jan 2023, at 01:39, 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.
> > > >
> > > > +
> > > > +/* Parse different TCP/IP protocol headers for a quick view inside an
> > > > mbuf. */
> > > > +void
> > > > +m_exract_headers(struct mbuf *mp, struct ether_header **eh, struct ip
> > > > **ip4,
> > > > + struct ip6_hdr **ip6, struct tcphdr **tcp, struct udphdr **udp)
> > > > +
> > >
> > > Should be m_extract_headers(). The rest of the diff looks good to me.
> > >
> >
> > Please wait.
> >
> > The mandatory nullification of `ip4', `ip6' and other variables passed
> > to m_exract_headers() is not obvious. It is much better to return
> > the integer result of extraction like m_tag_copy_chain() does.
>
> Yes, the mandatory nullification seems to be more errorprone. In my
> opinion is the number of results it not that useful. You have to check
> the retuned pointers anyway.
>
> I moved the nullification inside of m_exract_headers().
>
This is better. I also like the last return statement be removed from
m_extract_headers() before commit.
> 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 19 Jan 2023 09:29:10 -0000
> @@ -2477,23 +2477,18 @@ 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_header *eh;
> + struct ip *ip;
> + struct ip6_hdr *ip6;
> int offload = 0;
> uint32_t iphlen;
> uint8_t ipproto;
>
> - *vlan_macip_lens |= (sizeof(*eh) << IXGBE_ADVTXD_MACLEN_SHIFT);
> + m_extract_headers(mp, &eh, &ip, &ip6, NULL, NULL);
>
> - switch (ntohs(eh->ether_type)) {
> - case ETHERTYPE_IP: {
> - struct ip *ip;
> -
> - m = m_getptr(mp, sizeof(*eh), &hoff);
> - KASSERT(m != NULL && m->m_len - hoff >= sizeof(*ip));
> - ip = (struct ip *)(mtod(m, caddr_t) + hoff);
> + *vlan_macip_lens |= (sizeof(*eh) << IXGBE_ADVTXD_MACLEN_SHIFT);
>
> + if (ip) {
> iphlen = ip->ip_hl << 2;
> ipproto = ip->ip_p;
>
> @@ -2503,26 +2498,14 @@ 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);
> -
> + } else if (ip6) {
> iphlen = sizeof(*ip6);
> ipproto = ip6->ip6_nxt;
>
> *type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6;
> - break;
> - }
> #endif
> -
> - default:
> + } else {
> 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 19 Jan 2023 09:29:17 -0000
> @@ -2784,8 +2784,10 @@ 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_header *eh;
> + struct ip *ip;
> + struct ip6_hdr *ip6;
> + struct tcphdr *th;
> uint64_t hlen;
> uint8_t ipproto;
> uint64_t offload = 0;
> @@ -2800,39 +2802,23 @@ 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);
> + m_extract_headers(m0, &eh, &ip, &ip6, &th, NULL);
>
> + if (ip) {
> 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;
> - }
> -
> #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 (ip6) {
> offload |= IXL_TX_DESC_CMD_IIPT_IPV6;
>
> hlen = sizeof(*ip6);
> ipproto = ip6->ip6_nxt;
> - break;
> - }
> #endif
> - default:
> + } else {
> panic("CSUM_OUT set for non-IP packet");
> /* NOTREACHED */
> }
> @@ -2842,15 +2828,12 @@ ixl_tx_setup_offload(struct mbuf *m0)
>
> 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 (th == NULL)
> + break;
> +
> offload |= IXL_TX_DESC_CMD_L4T_EOFT_TCP;
> offload |= (uint64_t)th->th_off << IXL_TX_DESC_L4LEN_SHIFT;
> break;
> Index: kern/uipc_mbuf2.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_mbuf2.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 uipc_mbuf2.c
> --- kern/uipc_mbuf2.c 12 Dec 2020 11:48:54 -0000 1.45
> +++ kern/uipc_mbuf2.c 19 Jan 2023 09:27:44 -0000
> @@ -68,6 +68,10 @@
> #include <sys/pool.h>
> #include <sys/mbuf.h>
>
> +#include <net/if.h>
> +#include <net/if_var.h>
> +#include <netinet/if_ether.h>
> +
> extern struct pool mtagpool;
>
> /* can't call it m_dup(), as freebsd[34] uses m_dup() with different arg */
> @@ -386,4 +390,86 @@ struct m_tag *
> m_tag_next(struct mbuf *m, struct m_tag *t)
> {
> return (SLIST_NEXT(t, m_tag_link));
> +}
> +
> +/* Parse different TCP/IP protocol headers for a quick view inside an mbuf.
> */
> +void
> +m_extract_headers(struct mbuf *mp, struct ether_header **eh, struct ip **ip4,
> + struct ip6_hdr **ip6, struct tcphdr **tcp, struct udphdr **udp)
> +{
> + struct mbuf *m;
> + uint64_t hlen;
> + int hoff;
> + uint8_t ipproto;
> +
> + /* Return NULL if header was not recognized. */
> + if (eh != NULL)
> + *eh = NULL;
> + if (ip4 != NULL)
> + *ip4 = NULL;
> + if (ip6 != NULL)
> + *ip6 = NULL;
> + if (tcp != NULL)
> + *tcp = NULL;
> + if (udp != NULL)
> + *udp = NULL;
> +
> + if (eh == NULL)
> + return;
> +
> + *eh = mtod(mp, struct ether_header *);
> + switch (ntohs((*eh)->ether_type)) {
> + case ETHERTYPE_IP:
> + if (ip4 == NULL)
> + return;
> +
> + m = m_getptr(mp, sizeof(**eh), &hoff);
> + KASSERT(m != NULL && m->m_len - hoff >= sizeof(**ip4));
> + *ip4 = (struct ip *)(mtod(m, caddr_t) + hoff);
> +
> + hlen = (*ip4)->ip_hl << 2;
> + ipproto = (*ip4)->ip_p;
> +
> + break;
> +
> +#ifdef INET6
> + case ETHERTYPE_IPV6:
> + if (ip6 == NULL)
> + return;
> +
> + 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);
> +
> + hlen = sizeof(**ip6);
> + ipproto = (*ip6)->ip6_nxt;
> +
> + break;
> +#endif
> +
> + default:
> + return;
> + }
> +
> + switch (ipproto) {
> + case IPPROTO_TCP:
> + if (tcp == NULL)
> + return;
> +
> + m = m_getptr(m, hoff + hlen, &hoff);
> + KASSERT(m != NULL && m->m_len - hoff >= sizeof(**tcp));
> + *tcp = (struct tcphdr *)(mtod(m, caddr_t) + hoff);
> + break;
> +
> + case IPPROTO_UDP:
> + if (udp == NULL)
> + return;
> +
> + m = m_getptr(m, hoff + hlen, &hoff);
> + KASSERT(m != NULL && m->m_len - hoff >= sizeof(**udp));
> + *udp = (struct udphdr *)(mtod(m, caddr_t) + hoff);
> + break;
> + }
> +
> + return;
> }
> Index: sys/mbuf.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/mbuf.h,v
> retrieving revision 1.255
> diff -u -p -r1.255 mbuf.h
> --- sys/mbuf.h 15 Aug 2022 16:15:37 -0000 1.255
> +++ sys/mbuf.h 19 Jan 2023 09:28:04 -0000
> @@ -37,6 +37,14 @@
>
> #include <sys/queue.h>
>
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip6.h>
> +#include <netinet/tcp.h>
> +#include <netinet/udp.h>
> +
> +struct ether_header;
> +
> /*
> * Constants related to network buffer management.
> * MCLBYTES must be no larger than PAGE_SIZE (the software page size) and,
> @@ -466,6 +474,8 @@ int m_tag_copy_chain(struct mbuf *, stru
> void m_tag_init(struct mbuf *);
> struct m_tag *m_tag_first(struct mbuf *);
> struct m_tag *m_tag_next(struct mbuf *, struct m_tag *);
> +void m_extract_headers(struct mbuf *, struct ether_header **, struct ip **,
> + struct ip6_hdr **, struct tcphdr **, struct udphdr **);
>
> /* Packet tag types */
> #define PACKET_TAG_IPSEC_IN_DONE 0x0001 /* IPsec applied, in */
>
