On Thu, Jan 19, 2023 at 02:55:29PM +0300, Vitaliy Makkoveev wrote:
> 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.
Fixed below. Plus a suggestion from mpi to not pollute the namespace
with all the headers in mbuf.h. Moved them to uipc_mbuf2.c.
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 20:01:20 -0000
@@ -68,6 +68,15 @@
#include <sys/pool.h>
#include <sys/mbuf.h>
+#include <net/if.h>
+#include <net/if_var.h>
+#include <netinet/in.h>
+#include <netinet/if_ether.h>
+#include <netinet/ip.h>
+#include <netinet/ip6.h>
+#include <netinet/tcp.h>
+#include <netinet/udp.h>
+
extern struct pool mtagpool;
/* can't call it m_dup(), as freebsd[34] uses m_dup() with different arg */
@@ -386,4 +395,84 @@ 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;
+ }
}
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 19:57:43 -0000
@@ -467,6 +467,15 @@ 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 *);
+/* Parse different TCP/IP protocol headers */
+struct ether_header;
+struct ip;
+struct ip6_hdr;
+struct tcphdr;
+struct udphdr;
+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 */
#define PACKET_TAG_IPSEC_OUT_DONE 0x0002 /* IPsec applied, out */
