Re: use M_PROTO1 in bridge output too

2011-11-03 Thread Claudio Jeker
On Thu, Nov 03, 2011 at 01:21:25PM +0100, Camiel Dobbelaar wrote:
 No one interested in this one?  I have another bridge speedup diff after 
 this.
 

Ugh, forgot to answer on that one. I'm against overusing the M_PROTO1
flag. Keep in mind that M_PROTO1 is just a flag whereas the mbuf_tag is a
pointer to the ifp. So M_PROTO1 does not scale if there are multiple
bridges involved in forwarding a packet. M_PROTO1 is only OK to use when
the flag is cleared when leaving the protocol. E.g. it can be used when a
hint needs to be passed accross an ifq.

-- 
:wq Claudio

 On Fri, 28 Oct 2011, Camiel Dobbelaar wrote:
 
  M_PROTO1 is used by if_bridge on the input path.  On the output path it's 
  used now only by if_bridge for if_gif.  I think we can use it generically 
  to mark packets as processed by bridge in the output path.
  
  The diff simplifies things and avoids mtag checking and allocation so is 
  more efficient too.
  
  The old code checks if a packet has passed the _same_ bridge already, but 
  as an interface can only be a member of one bridge I think the flag is 
  sufficient.
  
  It looks like the only other user of M_PROTO1 is netbt/hci_link.c, but 
  that can be fixed if the diff is acceptable otherwise.
  
  Tested lightly in a bridge/gif setup, but could use some more testing.
  (especially with ipsec in the mix too)
  
  
  Index: if_bridge.c
  ===
  RCS file: /cvs/src/sys/net/if_bridge.c,v
  retrieving revision 1.193
  diff -u -p -r1.193 if_bridge.c
  --- if_bridge.c 4 Jul 2011 06:54:49 -   1.193
  +++ if_bridge.c 28 Oct 2011 17:55:04 -
  @@ -2813,9 +2813,8 @@ bridge_ifenqueue(struct bridge_softc *sc
   #if NGIF  0
  /* Packet needs etherip encapsulation. */
  if (ifp-if_type == IFT_GIF) {
  -   m-m_flags |= M_PROTO1;
  -
  /* Count packets input into the gif from outside */
  +   /* XXX do this in if_gif? */
  ifp-if_ipackets++;
  ifp-if_ibytes += m-m_pkthdr.len;
  }
  @@ -2844,6 +2843,7 @@ bridge_ifenqueue(struct bridge_softc *sc
  }
   #endif
  len = m-m_pkthdr.len;
  +   m-m_flags |= M_PROTO1;
  mflags = m-m_flags;
  IFQ_ENQUEUE(ifp-if_snd, m, NULL, error);
  if (error) {
  Index: if_ethersubr.c
  ===
  RCS file: /cvs/src/sys/net/if_ethersubr.c,v
  retrieving revision 1.151
  diff -u -p -r1.151 if_ethersubr.c
  --- if_ethersubr.c  9 Jul 2011 00:47:18 -   1.151
  +++ if_ethersubr.c  28 Oct 2011 17:55:04 -
  @@ -382,40 +382,8 @@ ether_output(ifp0, m0, dst, rt0)
   * Interfaces that are bridge members need special handling
   * for output.
   */
  -   if (ifp-if_bridge) {
  -   struct m_tag *mtag;
  -
  -   /*
  -* Check if this packet has already been sent out through
  -* this bridge, in which case we simply send it out
  -* without further bridge processing.
  -*/
  -   for (mtag = m_tag_find(m, PACKET_TAG_BRIDGE, NULL); mtag;
  -   mtag = m_tag_find(m, PACKET_TAG_BRIDGE, mtag)) {
  -#ifdef DEBUG
  -   /* Check that the information is there */
  -   if (mtag-m_tag_len != sizeof(caddr_t)) {
  -   error = EINVAL;
  -   goto bad;
  -   }
  -#endif
  -   if (!bcmp(ifp-if_bridge, mtag + 1, sizeof(caddr_t)))
  -   break;
  -   }
  -   if (mtag == NULL) {
  -   /* Attach a tag so we can detect loops */
  -   mtag = m_tag_get(PACKET_TAG_BRIDGE, sizeof(caddr_t),
  -   M_NOWAIT);
  -   if (mtag == NULL) {
  -   error = ENOBUFS;
  -   goto bad;
  -   }
  -   bcopy(ifp-if_bridge, mtag + 1, sizeof(caddr_t));
  -   m_tag_prepend(m, mtag);
  -   error = bridge_output(ifp, m, NULL, NULL);
  -   return (error);
  -   }
  -   }
  +   if (ifp-if_bridge  !(m-m_flags  M_PROTO1))
  +   return (bridge_output(ifp, m, NULL, NULL));
   #endif
  mflags = m-m_flags;
  len = m-m_pkthdr.len;



use M_PROTO1 in bridge output too

2011-10-28 Thread Camiel Dobbelaar
M_PROTO1 is used by if_bridge on the input path.  On the output path it's 
used now only by if_bridge for if_gif.  I think we can use it generically 
to mark packets as processed by bridge in the output path.

The diff simplifies things and avoids mtag checking and allocation so is 
more efficient too.

The old code checks if a packet has passed the _same_ bridge already, but 
as an interface can only be a member of one bridge I think the flag is 
sufficient.

It looks like the only other user of M_PROTO1 is netbt/hci_link.c, but 
that can be fixed if the diff is acceptable otherwise.

Tested lightly in a bridge/gif setup, but could use some more testing.
(especially with ipsec in the mix too)


Index: if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.193
diff -u -p -r1.193 if_bridge.c
--- if_bridge.c 4 Jul 2011 06:54:49 -   1.193
+++ if_bridge.c 28 Oct 2011 17:55:04 -
@@ -2813,9 +2813,8 @@ bridge_ifenqueue(struct bridge_softc *sc
 #if NGIF  0
/* Packet needs etherip encapsulation. */
if (ifp-if_type == IFT_GIF) {
-   m-m_flags |= M_PROTO1;
-
/* Count packets input into the gif from outside */
+   /* XXX do this in if_gif? */
ifp-if_ipackets++;
ifp-if_ibytes += m-m_pkthdr.len;
}
@@ -2844,6 +2843,7 @@ bridge_ifenqueue(struct bridge_softc *sc
}
 #endif
len = m-m_pkthdr.len;
+   m-m_flags |= M_PROTO1;
mflags = m-m_flags;
IFQ_ENQUEUE(ifp-if_snd, m, NULL, error);
if (error) {
Index: if_ethersubr.c
===
RCS file: /cvs/src/sys/net/if_ethersubr.c,v
retrieving revision 1.151
diff -u -p -r1.151 if_ethersubr.c
--- if_ethersubr.c  9 Jul 2011 00:47:18 -   1.151
+++ if_ethersubr.c  28 Oct 2011 17:55:04 -
@@ -382,40 +382,8 @@ ether_output(ifp0, m0, dst, rt0)
 * Interfaces that are bridge members need special handling
 * for output.
 */
-   if (ifp-if_bridge) {
-   struct m_tag *mtag;
-
-   /*
-* Check if this packet has already been sent out through
-* this bridge, in which case we simply send it out
-* without further bridge processing.
-*/
-   for (mtag = m_tag_find(m, PACKET_TAG_BRIDGE, NULL); mtag;
-   mtag = m_tag_find(m, PACKET_TAG_BRIDGE, mtag)) {
-#ifdef DEBUG
-   /* Check that the information is there */
-   if (mtag-m_tag_len != sizeof(caddr_t)) {
-   error = EINVAL;
-   goto bad;
-   }
-#endif
-   if (!bcmp(ifp-if_bridge, mtag + 1, sizeof(caddr_t)))
-   break;
-   }
-   if (mtag == NULL) {
-   /* Attach a tag so we can detect loops */
-   mtag = m_tag_get(PACKET_TAG_BRIDGE, sizeof(caddr_t),
-   M_NOWAIT);
-   if (mtag == NULL) {
-   error = ENOBUFS;
-   goto bad;
-   }
-   bcopy(ifp-if_bridge, mtag + 1, sizeof(caddr_t));
-   m_tag_prepend(m, mtag);
-   error = bridge_output(ifp, m, NULL, NULL);
-   return (error);
-   }
-   }
+   if (ifp-if_bridge  !(m-m_flags  M_PROTO1))
+   return (bridge_output(ifp, m, NULL, NULL));
 #endif
mflags = m-m_flags;
len = m-m_pkthdr.len;