On 01/07/20(Wed) 22:42, Vitaliy Makkoveev wrote: > pipex(4) has 2 mbuf queues: `pipexinq' and `pipexoutq'. When mbuf passed > to pipex it goes to one of these queues and pipexintr() will be > scheduled to process them. pipexintr() called from `netisr' context. > > It's true for pppac(4) but for pppx(4) only incoming mbufs go to > `pipexinq. Outgoing mbufs go directly to stack. pppx(4) enabled in > npppd.conf(5) by default so I guess it's the common case of pipex(4) > usage. > > The code looks like there is no requirements to this delayed mbufs > processing, we can pass it directly to stack as we do for pppx(4) > outgoing traffic. > > Also we have some troubles with pipexintr() as it was described in [1]. > It's protection of `ph_cookie'. We don't this protection this time and > we can't because we should brake if_get(9) logic. > > Diff below removes pipexintr(). Now all mbufs passed directly without > enqueueing within pipex(4). We also can destroy sessions safe in all > cases. We also can use if_get(9) instead using unreferenced pointers to > `ifnet' within pipex(4). We also avoided context switch while we > processing mbufs within pipex(4). We decreased latency. > > I'm seeding debian torrents with this diff an all goes well.
With this diff the content of pipexintr() is no longer executed with the KERNEL_LOCK() held. This can be seen by following the code starting in ether_input(). Grabbing the KERNEL_LOCK() there is not a way forward. The whole idea of if_input_process() is to be free of KERNEL_LOCK() to not introduce latency delay. So this changes implies that `pipex_session_list' and possibly other global data structures as well as the elements linked in those are all protected by the NET_LOCK(). I believe this is the easiest way forward. That said I would be comfortable with this diff going in if an audit of the data structures accessed in the code path starting by pipex_pppoe_input() has been done. That implies annotating/documenting which data structures are now protected by the NET_LOCK() and adding the necessary NET_ASSERT_LOCK(). Such audit might lead to consider some ioctl code path changes to now serialize on the NET_LOCK() instead of the KERNEL_LOCK(). > 1. https://marc.info/?t=159300809000002&r=1&w=2 > > Index: lib/libc/sys/sysctl.2 > =================================================================== > RCS file: /cvs/src/lib/libc/sys/sysctl.2,v > retrieving revision 1.40 > diff -u -p -r1.40 sysctl.2 > --- lib/libc/sys/sysctl.2 17 May 2020 05:48:39 -0000 1.40 > +++ lib/libc/sys/sysctl.2 1 Jul 2020 19:20:22 -0000 > @@ -2033,35 +2033,11 @@ The currently defined variable names are > .Bl -column "Third level name" "integer" "Changeable" -offset indent > .It Sy "Third level name" Ta Sy "Type" Ta Sy "Changeable" > .It Dv PIPEXCTL_ENABLE Ta integer Ta yes > -.It Dv PIPEXCTL_INQ Ta node Ta not applicable > -.It Dv PIPEXCTL_OUTQ Ta node Ta not applicable > .El > .Bl -tag -width "123456" > .It Dv PIPEXCTL_ENABLE > If set to 1, enable PIPEX processing. > The default is 0. > -.It Dv PIPEXCTL_INQ Pq Va net.pipex.inq > -Fourth level comprises an array of > -.Vt struct ifqueue > -structures containing information about the PIPEX packet input queue. > -The forth level names for the elements of > -.Vt struct ifqueue > -are the same as described in > -.Li ip.arpq > -in the > -.Dv PF_INET > -section. > -.It Dv PIPEXCTL_OUTQ Pq Va net.pipex.outq > -Fourth level comprises an array of > -.Vt struct ifqueue > -structures containing information about PIPEX packet output queue. > -The forth level names for the elements of > -.Vt struct ifqueue > -are the same as described in > -.Li ip.arpq > -in the > -.Dv PF_INET > -section. > .El > .El > .Ss CTL_VFS > Index: sys/net/if.c > =================================================================== > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.611 > diff -u -p -r1.611 if.c > --- sys/net/if.c 30 Jun 2020 09:31:38 -0000 1.611 > +++ sys/net/if.c 1 Jul 2020 19:20:27 -0000 > @@ -1012,13 +1012,6 @@ if_netisr(void *unused) > KERNEL_UNLOCK(); > } > #endif > -#ifdef PIPEX > - if (n & (1 << NETISR_PIPEX)) { > - KERNEL_LOCK(); > - pipexintr(); > - KERNEL_UNLOCK(); > - } > -#endif > t |= n; > } > > Index: sys/net/netisr.h > =================================================================== > RCS file: /cvs/src/sys/net/netisr.h,v > retrieving revision 1.51 > diff -u -p -r1.51 netisr.h > --- sys/net/netisr.h 6 Aug 2019 22:57:54 -0000 1.51 > +++ sys/net/netisr.h 1 Jul 2020 19:20:27 -0000 > @@ -48,7 +48,6 @@ > #define NETISR_IPV6 24 /* same as AF_INET6 */ > #define NETISR_ISDN 26 /* same as AF_E164 */ > #define NETISR_PPP 28 /* for PPP processing */ > -#define NETISR_PIPEX 27 /* for pipex processing */ > #define NETISR_BRIDGE 29 /* for bridge processing */ > #define NETISR_PPPOE 30 /* for pppoe processing */ > #define NETISR_SWITCH 31 /* for switch dataplane */ > @@ -68,7 +67,6 @@ void bridgeintr(void); > void pppoeintr(void); > void switchintr(void); > void pfsyncintr(void); > -void pipexintr(void); > > #define schednetisr(anisr) > \ > do { \ > Index: sys/net/pipex.c > =================================================================== > RCS file: /cvs/src/sys/net/pipex.c,v > retrieving revision 1.117 > diff -u -p -r1.117 pipex.c > --- sys/net/pipex.c 30 Jun 2020 14:05:13 -0000 1.117 > +++ sys/net/pipex.c 1 Jul 2020 19:20:28 -0000 > @@ -97,10 +97,6 @@ struct radix_node_head *pipex_rd_head6 = > struct timeout pipex_timer_ch; /* callout timer context */ > int pipex_prune = 1; /* walk list every seconds */ > > -/* pipex traffic queue */ > -struct mbuf_queue pipexinq = MBUF_QUEUE_INITIALIZER(IFQ_MAXLEN, IPL_NET); > -struct mbuf_queue pipexoutq = MBUF_QUEUE_INITIALIZER(IFQ_MAXLEN, IPL_NET); > - > /* borrow an mbuf pkthdr field */ > #define ph_ppp_proto ether_vtag > > @@ -713,82 +709,6 @@ pipex_lookup_by_session_id(int protocol, > } > > /*********************************************************************** > - * Queue and Software Interrupt Handler > - ***********************************************************************/ > -void > -pipexintr(void) > -{ > - struct pipex_session *pkt_session; > - u_int16_t proto; > - struct mbuf *m; > - struct mbuf_list ml; > - > - /* ppp output */ > - mq_delist(&pipexoutq, &ml); > - while ((m = ml_dequeue(&ml)) != NULL) { > - pkt_session = m->m_pkthdr.ph_cookie; > - if (pkt_session == NULL) { > - m_freem(m); > - continue; > - } > - proto = m->m_pkthdr.ph_ppp_proto; > - > - m->m_pkthdr.ph_cookie = NULL; > - m->m_pkthdr.ph_ppp_proto = 0; > - > - if (pkt_session->is_multicast != 0) { > - struct pipex_session *session; > - struct mbuf *m0; > - > - LIST_FOREACH(session, &pipex_session_list, > - session_list) { > - if (session->pipex_iface != > - pkt_session->pipex_iface) > - continue; > - if (session->ip_forward == 0 && > - session->ip6_forward == 0) > - continue; > - m0 = m_copym(m, 0, M_COPYALL, M_NOWAIT); > - if (m0 == NULL) { > - session->stat.oerrors++; > - continue; > - } > - pipex_ppp_output(m0, session, proto); > - } > - m_freem(m); > - } else > - pipex_ppp_output(m, pkt_session, proto); > - } > - > - /* ppp input */ > - mq_delist(&pipexinq, &ml); > - while ((m = ml_dequeue(&ml)) != NULL) { > - pkt_session = m->m_pkthdr.ph_cookie; > - if (pkt_session == NULL) { > - m_freem(m); > - continue; > - } > - pipex_ppp_input(m, pkt_session, 0); > - } > -} > - > -Static int > -pipex_ppp_enqueue(struct mbuf *m0, struct pipex_session *session, > - struct mbuf_queue *mq) > -{ > - m0->m_pkthdr.ph_cookie = session; > - /* XXX need to support other protocols */ > - m0->m_pkthdr.ph_ppp_proto = PPP_IP; > - > - if (mq_enqueue(mq, m0) != 0) > - return (1); > - > - schednetisr(NETISR_PIPEX); > - > - return (0); > -} > - > -/*********************************************************************** > * Timer functions > ***********************************************************************/ > Static void > @@ -840,13 +760,6 @@ pipex_timer(void *ignored_arg) > /* FALLTHROUGH */ > > case PIPEX_STATE_CLOSED: > - /* > - * mbuf queued in pipexinq or pipexoutq may have a > - * refererce to this session. > - */ > - if (!mq_empty(&pipexinq) || !mq_empty(&pipexoutq)) > - continue; > - > pipex_destroy_session(session); > break; > > @@ -947,8 +860,26 @@ pipex_ip_output(struct mbuf *m0, struct > m0->m_flags &= ~(M_BCAST|M_MCAST); > > /* output ip packets to the session tunnel */ > - if (pipex_ppp_enqueue(m0, session, &pipexoutq)) > - goto dropped; > + if (session->is_multicast != 0) { > + struct pipex_session *session_tmp; > + struct mbuf *m; > + > + LIST_FOREACH(session_tmp, &pipex_session_list, session_list) { > + if (session->pipex_iface != session_tmp->pipex_iface) > + continue; > + if (session->ip_forward == 0 && > + session->ip6_forward == 0) > + continue; > + m = m_copym(m0, 0, M_COPYALL, M_NOWAIT); > + if (m == NULL) { > + session->stat.oerrors++; > + continue; > + } > + pipex_ppp_output(m, session_tmp, PPP_IP); > + } > + m_freem(m); > + } else > + pipex_ppp_output(m0, session, PPP_IP); > > return; > drop: > @@ -1207,7 +1138,7 @@ pipex_ip6_input(struct mbuf *m0, struct > > Static struct mbuf * > pipex_common_input(struct pipex_session *session, struct mbuf *m0, int hlen, > - int plen, int useq) > + int plen) > { > int proto, ppphlen; > u_char code; > @@ -1255,19 +1186,10 @@ pipex_common_input(struct pipex_session > m_adj(m0, plen - m0->m_pkthdr.len); > } > > - if (!useq) { > - pipex_ppp_input(m0, session, 0); > - return (NULL); > - } > - > - /* input ppp packets to kernel session */ > - if (pipex_ppp_enqueue(m0, session, &pipexinq) != 0) > - goto dropped; > - else > - return (NULL); > + pipex_ppp_input(m0, session, 0); > + return (NULL); > drop: > m_freem(m0); > -dropped: > session->stat.ierrors++; > return (NULL); > > @@ -1363,7 +1285,7 @@ pipex_pppoe_input(struct mbuf *m0, struc > sizeof(struct pipex_pppoe_header), (caddr_t)&pppoe); > > hlen = sizeof(struct ether_header) + sizeof(struct pipex_pppoe_header); > - if ((m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length), 0)) > + if ((m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length))) > == NULL) > return (NULL); > m_freem(m0); > @@ -1658,7 +1580,7 @@ pipex_pptp_input(struct mbuf *m0, struct > pipex_pptp_output(NULL, session, 0, 1); > } > > - if ((m0 = pipex_common_input(session, m0, hlen, ntohs(gre->len), 1)) > + if ((m0 = pipex_common_input(session, m0, hlen, ntohs(gre->len))) > == NULL) { > /* ok, The packet is for PIPEX */ > if (!rewind) > @@ -2092,7 +2014,7 @@ pipex_l2tp_input(struct mbuf *m0, int of > > length -= hlen + offset; > hlen += off0 + offset; > - if ((m0 = pipex_common_input(session, m0, hlen, length, 1)) == NULL) { > + if ((m0 = pipex_common_input(session, m0, hlen, length)) == NULL) { > /* ok, The packet is for PIPEX */ > if (!rewind) > session->proto.l2tp.nr_gap += nseq; > @@ -3010,12 +2932,6 @@ pipex_sysctl(int *name, u_int namelen, v > return (ENOTDIR); > return (sysctl_int(oldp, oldlenp, newp, newlen, > &pipex_enable)); > - case PIPEXCTL_INQ: > - return (sysctl_mq(name + 1, namelen - 1, > - oldp, oldlenp, newp, newlen, &pipexinq)); > - case PIPEXCTL_OUTQ: > - return (sysctl_mq(name + 1, namelen - 1, > - oldp, oldlenp, newp, newlen, &pipexoutq)); > default: > return (ENOPROTOOPT); > } > Index: sys/net/pipex.h > =================================================================== > RCS file: /cvs/src/sys/net/pipex.h,v > retrieving revision 1.23 > diff -u -p -r1.23 pipex.h > --- sys/net/pipex.h 5 Jun 2020 19:50:59 -0000 1.23 > +++ sys/net/pipex.h 1 Jul 2020 19:20:28 -0000 > @@ -33,15 +33,11 @@ > * Names for pipex sysctl objects > */ > #define PIPEXCTL_ENABLE 1 > -#define PIPEXCTL_INQ 2 > -#define PIPEXCTL_OUTQ 3 > -#define PIPEXCTL_MAXID 4 > +#define PIPEXCTL_MAXID 2 > > #define PIPEXCTL_NAMES { \ > { 0, 0 }, \ > { "enable", CTLTYPE_INT }, \ > - { "inq", CTLTYPE_NODE }, \ > - { "outq", CTLTYPE_NODE }, \ > } > > #define PIPEX_PROTO_L2TP 1 /* protocol L2TP */ > Index: sys/net/pipex_local.h > =================================================================== > RCS file: /cvs/src/sys/net/pipex_local.h,v > retrieving revision 1.35 > diff -u -p -r1.35 pipex_local.h > --- sys/net/pipex_local.h 18 Jun 2020 14:20:12 -0000 1.35 > +++ sys/net/pipex_local.h 1 Jul 2020 19:20:28 -0000 > @@ -402,7 +402,8 @@ void pipex_ip_input (st > #ifdef INET6 > void pipex_ip6_input (struct mbuf *, struct pipex_session > *); > #endif > -struct mbuf *pipex_common_input(struct pipex_session *, struct > mbuf *, int, int, int); > +struct mbuf *pipex_common_input(struct pipex_session *, > + struct mbuf *, int, int); > > #ifdef PIPEX_PPPOE > void pipex_pppoe_output (struct mbuf *, struct > pipex_session *); >
