Re: Stop G/C mbufs in if_detach()

[Claudio Jeker]

On Tue, Jun 23, 2015 at 03:01:54PM +0200, Martin Pieuchot wrote:
 When an interface is detached or destroyed the CPU executing if_detach()
 removes all the mbufs received by this interface on three queues:
 ARP, IPv4 and IPv6 protocol queues.
 
 This made sense to avoid referencing a dangling rcvif pointer. But now
 mbufs contain unique interface indexes and protocol interrupt routines
 handle just fine the case where if_get() returns a NULL pointer.
 
 So I'd like to get rid of this explicit garbage collection.  Note that
 this will leave mbufs on the protocol queues until the next netisr is
 executed for the corresponding queue.  This is a functional change but
 I don't think it's a problem.  It only matters if you destroy your only
 pseudo interface or unplug your single USB interface without replugging
 it.
 
 Comments, oks?

OK claudio@ (there are many more queues that would need to be cleaned if
we wanted this)
 
 Index: net/if.c
 ===
 RCS file: /cvs/src/sys/net/if.c,v
 retrieving revision 1.341
 diff -u -p -r1.341 if.c
 --- net/if.c  23 Jun 2015 09:42:23 -  1.341
 +++ net/if.c  23 Jun 2015 10:54:04 -
 @@ -128,8 +128,6 @@ void  if_attachsetup(struct ifnet *);
  void if_attachdomain1(struct ifnet *);
  void if_attach_common(struct ifnet *);
  
 -int  if_detach_filter(void *, const struct mbuf *);
 -void if_detach_queues(struct ifnet *, struct niqueue *);
  void if_detached_start(struct ifnet *);
  int  if_detached_ioctl(struct ifnet *, u_long, caddr_t);
  
 @@ -644,23 +642,6 @@ if_detach(struct ifnet *ifp)
   pfi_detach_ifnet(ifp);
  #endif
  
 - /*
 -  * remove packets came from ifp, from software interrupt queues.
 -  * net/netisr_dispatch.h is not usable, as some of them use
 -  * strange queue names.
 -  */
 -#define IF_DETACH_QUEUES(x) \
 -do { \
 - extern struct niqueue x; \
 - if_detach_queues(ifp,  x); \
 -} while (0)
 - IF_DETACH_QUEUES(arpintrq);
 - IF_DETACH_QUEUES(ipintrq);
 -#ifdef INET6
 - IF_DETACH_QUEUES(ip6intrq);
 -#endif
 -#undef IF_DETACH_QUEUES
 -
   /* Remove the interface from the list of all interfaces.  */
   TAILQ_REMOVE(ifnet, ifp, if_list);
   if (ISSET(ifp-if_xflags, IFXF_TXREADY))
 @@ -701,34 +682,6 @@ do { \
  
   ifindex2ifnet[ifp-if_index] = NULL;
   splx(s);
 -}
 -
 -int
 -if_detach_filter(void *ctx, const struct mbuf *m)
 -{
 - struct ifnet *ifp = ctx;
 -
 -#ifdef DIAGNOSTIC
 - if ((m-m_flags  M_PKTHDR) == 0)
 - return (0);
 -#endif
 -
 - return (m-m_pkthdr.ph_ifidx == ifp-if_index);
 -}
 -
 -void
 -if_detach_queues(struct ifnet *ifp, struct niqueue *niq)
 -{
 - struct mbuf *m0, *m;
 -
 - m0 = niq_filter(niq, if_detach_filter, ifp);
 - while (m0 != NULL) {
 - m = m0;
 - m0 = m-m_nextpkt;
 -
 - m-m_nextpkt = NULL;
 - m_freem(m);
 - }
  }
  
  /*
 

-- 
:wq Claudio

Stop G/C mbufs in if_detach()

[Martin Pieuchot]

When an interface is detached or destroyed the CPU executing if_detach()
removes all the mbufs received by this interface on three queues:
ARP, IPv4 and IPv6 protocol queues.

This made sense to avoid referencing a dangling rcvif pointer. But now
mbufs contain unique interface indexes and protocol interrupt routines
handle just fine the case where if_get() returns a NULL pointer.

So I'd like to get rid of this explicit garbage collection.  Note that
this will leave mbufs on the protocol queues until the next netisr is
executed for the corresponding queue.  This is a functional change but
I don't think it's a problem.  It only matters if you destroy your only
pseudo interface or unplug your single USB interface without replugging
it.

Comments, oks?

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.341
diff -u -p -r1.341 if.c
--- net/if.c23 Jun 2015 09:42:23 -  1.341
+++ net/if.c23 Jun 2015 10:54:04 -
@@ -128,8 +128,6 @@ voidif_attachsetup(struct ifnet *);
 void   if_attachdomain1(struct ifnet *);
 void   if_attach_common(struct ifnet *);
 
-intif_detach_filter(void *, const struct mbuf *);
-void   if_detach_queues(struct ifnet *, struct niqueue *);
 void   if_detached_start(struct ifnet *);
 intif_detached_ioctl(struct ifnet *, u_long, caddr_t);
 
@@ -644,23 +642,6 @@ if_detach(struct ifnet *ifp)
pfi_detach_ifnet(ifp);
 #endif
 
-   /*
-* remove packets came from ifp, from software interrupt queues.
-* net/netisr_dispatch.h is not usable, as some of them use
-* strange queue names.
-*/
-#define IF_DETACH_QUEUES(x) \
-do { \
-   extern struct niqueue x; \
-   if_detach_queues(ifp,  x); \
-} while (0)
-   IF_DETACH_QUEUES(arpintrq);
-   IF_DETACH_QUEUES(ipintrq);
-#ifdef INET6
-   IF_DETACH_QUEUES(ip6intrq);
-#endif
-#undef IF_DETACH_QUEUES
-
/* Remove the interface from the list of all interfaces.  */
TAILQ_REMOVE(ifnet, ifp, if_list);
if (ISSET(ifp-if_xflags, IFXF_TXREADY))
@@ -701,34 +682,6 @@ do { \
 
ifindex2ifnet[ifp-if_index] = NULL;
splx(s);
-}
-
-int
-if_detach_filter(void *ctx, const struct mbuf *m)
-{
-   struct ifnet *ifp = ctx;
-
-#ifdef DIAGNOSTIC
-   if ((m-m_flags  M_PKTHDR) == 0)
-   return (0);
-#endif
-
-   return (m-m_pkthdr.ph_ifidx == ifp-if_index);
-}
-
-void
-if_detach_queues(struct ifnet *ifp, struct niqueue *niq)
-{
-   struct mbuf *m0, *m;
-
-   m0 = niq_filter(niq, if_detach_filter, ifp);
-   while (m0 != NULL) {
-   m = m0;
-   m0 = m-m_nextpkt;
-
-   m-m_nextpkt = NULL;
-   m_freem(m);
-   }
 }
 
 /*