Page MenuHomeFreeBSD

ktls: switch bare zone_mbuf use to m_free
ClosedPublic

Authored by mjg on Jun 30 2021, 1:07 PM.

Details

Summary

The intent is to make the zone_mbuf static and switch from ctor/dtor through an indirect function call to a wrapper around uma_zalloc.

I'm not sure about the extra checks which come with it:

if ((m->m_flags & (M_PKTHDR|M_NOFREE)) == (M_PKTHDR|M_NOFREE))
        m_tag_delete_chain(m, NULL);
if (m->m_flags & M_PKTHDR && m->m_pkthdr.csum_flags & CSUM_SND_TAG)
        m_snd_tag_rele(m->m_pkthdr.snd_tag);
if (m->m_flags & M_EXTPG)
        mb_free_extpg(m);
else if (m->m_flags & M_EXT)
        mb_free_ext(m);
else if ((m->m_flags & M_NOFREE) == 0)
        uma_zfree(zone_mbuf, m);

I presume M_PKTHDR is not set, but I have trouble figuring out M_EXTPG and M_EXT and I don't have any means to test this. Worst case I can provide m_free_raw which ends up being an equivalent of doing uma_zfree(zone_mbuf, m);.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mjg requested review of this revision.Jun 30 2021, 1:07 PM
mjg created this revision.

I think I'd prefer an m_free_raw(). I think that if we switched to m_free(), we'd wind up bringing in the first cache line of the mbuf (for the flags checks). If I'm wrong about that, and you see something else that will bring in that cache line already, then I'm fine with m_free.

Thanks for pointing out the dtor checks the flags already, thats a bummer. So this makes things no worse, except for some tests which i think is fine.

This revision is now accepted and ready to land.Jun 30 2021, 1:25 PM

destructor called by uma brings this in:

static void
mb_dtor_mbuf(void *mem, int size, void *arg)
{       
        struct mbuf *m;  
        unsigned long flags;
        
        m = (struct mbuf *)mem;
        flags = (unsigned long)arg;
        
        KASSERT((m->m_flags & M_NOFREE) == 0, ("%s: M_NOFREE set", __func__));
        KASSERT((flags & 0x1) == 0, ("%s: obsolete MB_DTOR_SKIP passed", __func__));
        if ((m->m_flags & M_PKTHDR) && !SLIST_EMPTY(&m->m_pkthdr.tags))
                m_tag_delete_chain(m, NULL);
}

This behavior will stop after I introduce dedicated _pkthdr variants.

Here I'm just worried about the flags I mentioned. That said, if you could test this in your setup (with an assert that neither M_EXTPG nor M_EXT are set) that would be great. This change is a blocker to a revamp how zone_mbuf is handled.

mjg planned changes to this revision.Jun 30 2021, 1:42 PM

15:32 < fbsdslack> <gallatin> the flags will be 0x109, so you'll need to clear them
15:32 < fbsdslack> <gallatin> (from dtracing ktls_enqueue_to_send() on a live system)

These evaluate to M_EXT, M_RDONLY and M_EXTPG.

  • use m_free_raw instead
This revision is now accepted and ready to land.Jun 30 2021, 5:23 PM
This revision was automatically updated to reflect the committed changes.