Re: CFT: vr(4)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Pyun,

I've ported your driver to DragonFlyBSD and am running it on a PC Engines ALIX1.C
board (www.pcengines.ch) which has this nic -


vr0: <VIA VT6105M Rhine III 10/100BaseTX> port 0xfc00-0xfcff mem 0xeffff000-0xeffff0ff irq 10 at device 13.0 on pci0
vr0: Quirks: 0x6
vr0: Revision: 0x96
vr0: MAC address: 00:0d:b9:0c:d6:00

vr0_at_pci0:13:0:  class=0x020000 card=0x01061106 chip=0x30531106 rev=0x96 hdr=0x00
    vendor   = 'VIA Technologies Inc'
    device   = 'VT6105M Rhine III Management Adapter'
    class    = network
    subclass = ethernet

Your driver is slightly faster than the old vr driver. I'm using netstrain/netstraind to measure
throughput. I also find that interrupt load is a little bit higher than the old driver, but maybe
that is because throughput is also higher.

One of the problems that I ran into with this new driver was Tx/Rx buffer alignment. It seems that
my chip needs longword alignment for both transmit and receive buffers.. So I added the VR_Q_NEEDALIGN
to the quirks table and then saw -

vr0: Quirks: 0x7

But that didn't fix the problem.  I noticed that in vr_newbuf you have the following code -

        m->m_len = m->m_pkthdr.len = MCLBYTES;
        m_adj(m, sizeof(uint64_t));

        error = bus_dmamap_load_mbuf_sg(..

I'm not sure of the purpose of the call to m_adj here, but is not providing longword alignment -
it is just adding some padding.

Similarly in vr_encap(), you have the following code -

       if ((sc->vr_quirks & VR_Q_NEEDALIGN) != 0) {
                m = m_defrag(*m_head, MB_DONTWAIT);
                if (m == NULL) {
                        m_freem(*m_head);
                        *m_head = NULL;
                        return (ENOBUFS);
                }
                *m_head = m;
        }

In DragonFlyBSD at least doing an m_defrag does not guarantee longword alignment - and it's slow.

So, for DragonFly, I added this routine - kind of unelegant, but functional -


/* try and fixup longword alignment */
static __inline void
vr_fixup_lw(struct mbuf *m)
{
        uint8_t         *src, *dst;
        int                     i;
        src = mtod(m, uint8_t *);

        if ( (dst = (uint8_t *)((int)src & ~(sizeof(uint64_t)-1))) == 0 )
                return;

        if ( M_LEADINGSPACE(m) < (dst - src) ) {
                kprintf("vr_fixup_lw: cannot repair!\n");
                return;
        }

        m->m_data = dst;

        for (i = 0; i < m->m_len; i++)
                *dst++ = *src++;
}


Now, in vr_encap() I instead have the following -

       if ((sc->vr_quirks & VR_Q_NEEDALIGN) != 0)
		vr_fixed_lw(m);


and in vr_newbuf() -

        m->m_len = m->m_pkthdr.len = MCLBYTES;
        m_adj(m, sizeof(uint64_t));

	if ((sc->vr_quirks & VR_Q_NEEDALIGN) != 0)
		vr_fixup_lw(m);

        error = bus_dmamap_load_mbuf_sg(..


These two changes nicely fix my alignment issues. :o)


Finally I had a problem with my phy device .. it appeared to be wedged, I
could receive but not send.

The problem seemed to be in vr_init_locked(), towards the bottom. You had -

	sc->vr_link = 0;
	mii_mediachg(mii)

	ifp->if...

but this didn't work. I discovered that running -

# ifconfig vr0 media auto

will correct the problem, and so I added the phy_resets() found in vr_ifmedia_upd()
into vr_init_locked(), so the new code looks like -

	sc->vr_link = 0;
        if (mii->mii_instance) {
                struct mii_softc *miisc;
                LIST_FOREACH(miisc, &mii->mii_phys, mii_list)
                        mii_phy_reset(miisc);
        }
        mii_mediachg(mii);


Normally I would provide a diff against FreeBSD sources, but I have no place to easily
test this on FreeBSD, and these 4 changes (if you count the change to the quirks table)
are quite small.


Nice job on the driver!

- --Andrew.


Pyun YongHyeon wrote:
> Dear all,
> 
> Here is overhauled vr(4) that shall address all known issues. PR
> database showed vr(4) is not stable enough under high load and
> link state handling didn't work as expected as well as its poor
> performance. I've tried hard to fix the bugs reported in PR
> database for several months. It tooked more time than I had
> planned when I received donated hardware.
> vr(4) will now
>  - work on all architectures.
>  - have better Rx performance.
>  - have more robust error recovery.
>  - reliably detect link state changes.
>  - support 32bit perfect multicast filtering for VT6105M.
>  - support WOL.
> 
> It seems that the overhauled vr(4) runs well under my limited test
> environments. Several users already reported success. Because there
> are three kinds of Rhine family and lots of variants for the
> hardware, I'd like to hear feedback from users priror to commit.
> You can get the latest vr(4) at the following URL.
> 
> http://people.freebsd.org/~yongari/vr/if_vr.c
> http://people.freebsd.org/~yongari/vr/if_vrreg.h
> 
> I wouldn't be available for 7 ~ 10 days from Feb 5, so please don't
> expect quick reply.
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.4 (FreeBSD)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHsokP8It2CaCdeMwRAjGTAJ9Plc/+i3GWor1uGgsnTihKWEezcgCeOyTP
alM6RjNsSjxUk/4DKdIQuds=
=hdHZ
-----END PGP SIGNATURE-----