On Thu, Oct 06, 2016 at 10:11 +0900, YASUOKA Masahiko wrote:
>
> On Wed, 5 Oct 2016 14:30:27 +0200
> Mike Belopuhov <m...@belopuhov.com> wrote:
> > On Wed, Oct 05, 2016 at 10:58 +0900, YASUOKA Masahiko wrote:
> >> On Tue, 4 Oct 2016 17:27:12 +0200
> >> Mike Belopuhov <m...@belopuhov.com> wrote:
> >> > On Tue, Oct 04, 2016 at 01:07 +0200, Vincent Gross wrote:
> >> >> On Sat, 24 Sep 2016 10:58:10 +0200
> >> >> Vincent Gross <vgr...@openbsd.org> wrote:
> >> >>
> >> >> > Hi,
> >> >> >
> >> >> [snip]
> >> >> >
> >> >> > Aside from the mbuf issue, is this Ok ?
> >> >>
> >> >> I will go back on the mbuff stuff later.
> >> >>
> >> >> Diff rebased, ok anyone ?
> >> >>
> >> >> Index: net/if_vxlan.c
> >> >> ===================================================================
> >> >> RCS file: /cvs/src/sys/net/if_vxlan.c,v
> >> >> retrieving revision 1.48
> >> >> diff -u -p -r1.48 if_vxlan.c
> >> >> --- net/if_vxlan.c 30 Sep 2016 10:22:05 -0000 1.48
> >> >> +++ net/if_vxlan.c 3 Oct 2016 23:12:42 -0000
> >> >> @@ -638,7 +749,9 @@ vxlan_lookup(struct mbuf *m, struct udph
> >> >> if (m->m_pkthdr.len < skip + sizeof(struct ether_header))
> >> >> return (EINVAL);
> >> >>
> >> >> - m_adj(m, skip);
> >> >> + m_adj(m, skip - ETHER_ALIGN);
> >> >> + m = m_pullup(m, ETHER_HDR_LEN + ETHER_ALIGN);
> >> >> + m_adj(m, ETHER_ALIGN);
> >> >> ifp = &sc->sc_ac.ac_if;
> >> >>
> >> >> #if NBRIDGE > 0
> >> >
> >> > I think this chunk is correct. First you ensure that m->m_data
> >> > points to a contiguous and well-aligned ethernet header and then
> >> > you trim the alignment so that mtod() points directly at it.
> >>
> >> Isn't it possible that m_pullup() may return non aligned pointer?
> >>
> >
> > Do you mean if m->m_data pointer can be not aligned properly after
> > an m_pullup?
>
> Sorry. Yes, I meant that.
>
> > Of course, if the header layout is such that m_pullup doesn't need
> > to move data around and at the same time the header that we're going
> > to m_adj[ust] to is not aligned properly, this is going to be a
> > problem.
>
> The diff I sent to hackers@ is including my lastest proposal
>
> @@ -657,6 +658,16 @@ vxlan_lookup(struct mbuf *m, struct udph
> #if NPF > 0
> pf_pkt_addr_changed(m);
> #endif
> + if ((m = m_pullup(m, sizeof(struct ether_header))) == NULL)
> + return (ENOBUFS);
> +
> + if (!ALIGNED_POINTER(mtod(m, caddr_t) + ETHER_HDR_LEN, uint32_t)) {
> + m0 = m;
> + m = m_dup_pkt(m0, ETHER_ALIGN, M_NOWAIT);
> + m_freem(m0);
> + if (m == NULL)
> + return (ENOBUFS);
> + }
>
> ml_enqueue(&ml, m);
> if_input(ifp, &ml);
>
>
> We might have a better way, but I think above diff does what we need
> to do here simply.
>
> 1. ensure entire ethernet header is on first mbuf for ether_input()
> 2. align 32bit at the ethernet payload (eg. IP)
>
> --yasuoka
It does and I've considered this, but this penalises the normal case
(2k cluster) too much. m_pullup will always get a new mbuf and on
strict alignment architectures you will always do a m_dup_pkt
(verified by my -DTEST1).
