Bug 4020 - vxget() in /sys/dev/vx/if_vx.c needs rework
Summary: vxget() in /sys/dev/vx/if_vx.c needs rework
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 2.2.1-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: itojun
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 1997-07-03 14:00 UTC by Jun-ichiro itojun Hagino
Modified: 1997-10-01 07:01 UTC (History)
0 users

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jun-ichiro itojun Hagino 1997-07-03 14:00:01 UTC 
vxget() in if_vx.c almost always store packet into multiple mbufs.
It makes upper-layer protocol processing slightly slower.
The code looks awful (I'm not attacking the author, sorry) and
can be simplified/can be made more readable.

it would be better if:
	packet > MHLEN to be put into single external mbuf
	otherwise store into single internal mbuf


The problem still remains in the latest source in CVS branch.

Fix: 

I'll try it later if nobody does it.
How-To-Repeat: none
Comment 1 Jun-ichiro itojun Hagino 1997-07-03 17:34:13 UTC 
	how about this?
	(if_vx.c- is based on 2.2.1-RELEASE source code)

itojun


---
*** if_vx.c-	Fri Jul  4 09:39:38 1997
--- if_vx.c	Fri Jul  4 10:33:36 1997
***************
*** 820,832 ****
  }
  
  static struct mbuf *
! vxget(sc, totlen)
      struct vx_softc *sc;
!     u_int totlen;
  {
      struct ifnet *ifp = &sc->arpcom.ac_if;
!     struct mbuf *top, **mp, *m;
!     int len;
      int sh;
  
      m = sc->mb[sc->next_mb];
--- 820,831 ----
  }
  
  static struct mbuf *
! vxget(sc, len)
      struct vx_softc *sc;
!     u_int len;
  {
      struct ifnet *ifp = &sc->arpcom.ac_if;
!     struct mbuf *m;
      int sh;
  
      m = sc->mb[sc->next_mb];
***************
*** 845,854 ****
          m->m_flags = M_PKTHDR;
      }
      m->m_pkthdr.rcvif = ifp;
!     m->m_pkthdr.len = totlen;
!     len = MHLEN;
!     top = 0;
!     mp = ⊤
  
      /*
       * We read the packet at splhigh() so that an interrupt from another
--- 844,850 ----
          m->m_flags = M_PKTHDR;
      }
      m->m_pkthdr.rcvif = ifp;
!     m->m_pkthdr.len = len;
  
      /*
       * We read the packet at splhigh() so that an interrupt from another
***************
*** 857,902 ****
       */
      sh = splhigh();
  
!     while (totlen > 0) {
!         if (top) {
!             m = sc->mb[sc->next_mb];
!             sc->mb[sc->next_mb] = 0;
!             if (m == 0) {
!                 MGET(m, M_DONTWAIT, MT_DATA);
!                 if (m == 0) {
!                     splx(sh);
!                     m_freem(top);
!                     return 0;
!                 }
!             } else {
!                 sc->next_mb = (sc->next_mb + 1) % MAX_MBS;
!             }
!             len = MLEN;
!         }
!         if (totlen >= MINCLSIZE) {
!         MCLGET(m, M_DONTWAIT);
!         if (m->m_flags & M_EXT)
!             len = MCLBYTES;
!         }
!         len = min(totlen, len);
!         if (len > 3) {
!             len &= ~3;
!             insl(BASE + VX_W1_RX_PIO_RD_1, mtod(m, u_int32_t *),
!                 len / 4);
!         } else
!             insb(BASE + VX_W1_RX_PIO_RD_1, mtod(m, u_int8_t *),
!                 len);
!         m->m_len = len;
!         totlen -= len;
!         *mp = m;
!         mp = &m->m_next;
      }
  
      outw(BASE +VX_COMMAND, RX_DISCARD_TOP_PACK);
  
      splx(sh);
  
!     return top;
  }
  
  
--- 853,881 ----
       */
      sh = splhigh();
  
!     if (MHLEN < len) {
! 	/* assumes ETHERMTU < MCLBYTES */
! 	MCLGET(m, M_DONTWAIT);
! 	if (! (m->m_flags & M_EXT)) {
! 	    m_freem(m);
! 	    goto fail;
! 	}
!     }
!     m->m_len = len;
!     insl(BASE + VX_W1_RX_PIO_RD_1, mtod(m, u_int32_t *), len / 4);
!     if (len & 3) {
! 	insb(BASE + VX_W1_RX_PIO_RD_1, mtod(m, u_int8_t *) + (len & ~3),
! 	    len & 3);
      }
  
      outw(BASE +VX_COMMAND, RX_DISCARD_TOP_PACK);
  
      splx(sh);
+     return m;
  
! fail:
!     splx(sh);
!     return NULL;
  }
Comment 2 davidg freebsd_committer freebsd_triage 1997-07-26 00:36:08 UTC 
Responsible Changed
From-To: freebsd-bugs->itojun

Assigned to itojun. I have no problem with the proposed change as long as 
the test is changed from (MHLEN < len) to (len > MHLEN).
Comment 3 Jun-ichiro itojun Hagino 1997-08-05 05:12:42 UTC 
just to announce the current situation.
i've got my patch reviewed by the author, however my main
machine crushed (motherboard dead?) so i can't patch/test/commit it
myself right now. pls hold for few days.

itojun
Comment 4 itojun freebsd_committer freebsd_triage 1997-10-01 06:59:30 UTC 
State Changed
From-To: open->closed


finally committed as src/sys/dev/vx/if_vx.c, 1.10 -> 1.11. 
(RELENG_2_2 1.2.2.3 -> 1.2.2.4) 
sorry that it took too much time.