Resend with unified diffs, sorry...
On 28 March 2011 20:11, Ben Gould <ben.go...@gmail.com> wrote:
> Hi,
>
> A bug fix, performance improvement and a new feature for pppoe(8); and
> a performance improvement for ppp(8).
>
> pppoe(8):
>
> - Under load my low power atom-based router kept hitting the
> "usleep(100000);" case in pppoe which seemed to throw LCP echos off
> causing line drops, this is now gone and the line doesn't drop.
> - select() replaced for kqueue()/kevent() calls in pppoe's client loop.
> - RFC 4638 support added to pppoe's client mode for negotiating 1500
> byte mtu with the server.
>
> ppp(8)
>
> - A very quick and dirty drop-in kqueue()/kevent() replacement for
> select() in the main loop. B It works for me, works well with the above
> pppoe mods, you'll flame. B Suggestions would be welcome. B Can anyone
> test this with dialup?
>
> Some background for these changes: B Faster broadband got rolled out
> here; all was working on ADSL 2+ (with random but tolerable line drops
> during bittorrent downloads). B Upgraded to VDSL (here in the UK BT are
> rolling out fibre to a roadside cabinet...) which is provided as a
> PPPoE mode. B Line now drops were happening under normal usage so I
> investigated... B Once all that was fixed I thought I'd add support for
> the larger MTU. B I probably wouldn't have needed to rewrite the
> select() calls as kqueue()/kevent() if I had a faster computer acting
> as a router. B pppoe(4) seems to cause panics and wasn't setting up
> IPv6 properly for me - sorry I've not investigated that further.
>
> Diffs and dmesg follow.
>
> -- ben
diff -upr ppp/ppp/main.c ppp.kqueue/ppp/main.c
--- ppp/ppp/main.c Wed Nov 12 16:01:08 2008
+++ ppp.kqueue/ppp/main.c Mon Mar 28 19:14:59 2011
@@ -48,6 +48,8 @@
#include <termios.h>
#include <unistd.h>
#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/event.h>
#ifndef NONAT
#ifdef LOCALNAT
@@ -533,27 +535,47 @@ main(int argc, char **argv)
static void
DoLoop(struct bundle *bundle)
{
- fd_set *rfds, *wfds, *efds;
- int i, nfds, nothing_done;
+ int kq;
+ struct kevent *changelist, *pevent;
+ fd_set *rfds, *wfds, *efds;
+ int i, nfds, nfds_done, nothing_done, x, nchanges, maxnchanges, neret;
- if ((rfds = mkfdset()) == NULL) {
- log_Printf(LogERROR, "DoLoop: Cannot create fd_set\n");
- return;
- }
+ nfds_done = 0;
+ nchanges = 0;
+ maxnchanges = getdtablesize();
+ changelist = malloc(maxnchanges * sizeof(struct kevent));
- if ((wfds = mkfdset()) == NULL) {
- log_Printf(LogERROR, "DoLoop: Cannot create fd_set\n");
- free(rfds);
- return;
- }
+ kq = kqueue();
+ if (kq < 0) {
+ log_Printf(LogERROR, "DoLoop: Cannot create kqueue\n");
+ free(changelist);
+ return;
+ }
- if ((efds = mkfdset()) == NULL) {
- log_Printf(LogERROR, "DoLoop: Cannot create fd_set\n");
- free(rfds);
- free(wfds);
- return;
- }
+ if ((rfds = mkfdset()) == NULL) {
+ log_Printf(LogERROR, "DoLoop: Cannot create fd_set\n");
+ free(changelist);
+ close(kq);
+ return;
+ }
+ if ((wfds = mkfdset()) == NULL) {
+ log_Printf(LogERROR, "DoLoop: Cannot create fd_set\n");
+ free(changelist);
+ close(kq);
+ free(rfds);
+ return;
+ }
+
+ if ((efds = mkfdset()) == NULL) {
+ log_Printf(LogERROR, "DoLoop: Cannot create fd_set\n");
+ free(changelist);
+ close(kq);
+ free(rfds);
+ free(wfds);
+ return;
+ }
+
for (; !bundle_IsDead(bundle); bundle_CleanDatalinks(bundle)) {
nfds = 0;
zerofdset(rfds);
@@ -571,6 +593,28 @@ DoLoop(struct bundle *bundle)
/* Don't select - we'll be here forever */
break;
+ /* Quick and dirty way to convert a complicated use of
+ * select into a quicker call to kevent(2)
+ */
+ nfds_done = nfds > nfds_done ? nfds : nfds_done;
+ for (x = 0; x < nfds; ++x) {
+ int r = 0, w = 0;
+ if (FD_ISSET(x, rfds)) {
+ EV_SET(changelist + nchanges, x, EVFILT_READ,
EV_ADD|EV_ONESHOT, 0, 0,
0);
+ ++nchanges;
+ r = 1;
+ }
+ if (FD_ISSET(x, wfds)) {
+ EV_SET(changelist + nchanges, x, EVFILT_WRITE,
EV_ADD|EV_ONESHOT, 0, 0,
0);
+ ++nchanges;
+ w = 1;
+ }
+ if (!r && !w && FD_ISSET(x, efds)) {
+ EV_SET(changelist + nchanges, x, EVFILT_READ,
EV_ADD|EV_ONESHOT, 0, 0,
0);
+ ++nchanges;
+ }
+ }
+
/*
* It's possible that we've had a signal since we last checked. If
* we don't check again before calling select(), we may end up stuck
@@ -582,48 +626,36 @@ DoLoop(struct bundle *bundle)
if (sig_Handle())
continue;
- i = select(nfds, rfds, wfds, efds, NULL);
+ i = neret = kevent(kq, changelist, nchanges, changelist, maxnchanges,
NULL);
+ nchanges = 0;
- if (i < 0 && errno != EINTR) {
- log_Printf(LogERROR, "DoLoop: select(): %s\n", strerror(errno));
- if (log_IsKept(LogTIMER)) {
- struct timeval t;
+ if (i < 0 && errno != EINTR) {
+ log_Printf(LogERROR, "DoLoop: select(): %s\n", strerror(errno));
+ break;
+ }
- for (i = 0; i <= nfds; i++) {
- if (FD_ISSET(i, rfds)) {
- log_Printf(LogTIMER, "Read set contains %d\n", i);
- FD_CLR(i, rfds);
- t.tv_sec = t.tv_usec = 0;
- if (select(nfds, rfds, wfds, efds, &t) != -1) {
- log_Printf(LogTIMER, "The culprit !\n");
- break;
- }
- }
- if (FD_ISSET(i, wfds)) {
- log_Printf(LogTIMER, "Write set contains %d\n", i);
- FD_CLR(i, wfds);
- t.tv_sec = t.tv_usec = 0;
- if (select(nfds, rfds, wfds, efds, &t) != -1) {
- log_Printf(LogTIMER, "The culprit !\n");
- break;
- }
- }
- if (FD_ISSET(i, efds)) {
- log_Printf(LogTIMER, "Error set contains %d\n", i);
- FD_CLR(i, efds);
- t.tv_sec = t.tv_usec = 0;
- if (select(nfds, rfds, wfds, efds, &t) != -1) {
- log_Printf(LogTIMER, "The culprit !\n");
- break;
- }
- }
- }
- }
- break;
- }
+ zerofdset(rfds);
+ zerofdset(wfds);
+ zerofdset(efds);
+ if (neret > 0) {
+ for (x = 0, pevent = changelist; x < neret; ++x, ++pevent) {
+ if (pevent->filter == EVFILT_READ) {
+ if (pevent->flags & EV_ERROR) {
+ FD_SET(pevent->ident, efds);
+ } else if (pevent->data > 0) {
FD_SET(pevent->ident, rfds); }
+ }
+ else if (pevent->filter == EVFILT_WRITE) {
+ if (pevent->flags & EV_ERROR) {
+ FD_SET(pevent->ident, efds);
+ } else if (pevent->data > 0) {
FD_SET(pevent->ident, wfds); }
+ }
+ }
+ }
- log_Printf(LogTIMER, "Select returns %d\n", i);
+ /* Note: select() error vs read/write diagnostics removed */
+ log_Printf(LogTIMER, "kevent returns %d\n", i);
+
sig_Handle();
if (i <= 0)
@@ -662,13 +694,14 @@ DoLoop(struct bundle *bundle)
* going back immediately to do our UpdateSet()s and select(),
* we sleep for a bit to avoid gobbling up all cpu time.
*/
- struct timeval t;
-
- t.tv_sec = 0;
- t.tv_usec = 100000;
- select(0, NULL, NULL, NULL, &t);
+ usleep(1);
}
}
log_Printf(LogDEBUG, "DoLoop done.\n");
+ free(changelist);
+ close(kq);
+ free(rfds);
+ free(wfds);
+ free(efds);
}
diff -upr pppoe/client.c pppoe.kqueue/client.c
--- pppoe/client.c Mon Sep 3 15:26:54 2007
+++ pppoe.kqueue/client.c Sun Mar 27 21:27:29 2011
@@ -47,6 +47,7 @@
#include <sysexits.h>
#include <stdlib.h>
#include <signal.h>
+#include <sys/event.h>
#include "pppoe.h"
@@ -56,6 +57,7 @@
u_int32_t client_cookie;
u_int16_t client_sessionid;
+u_int16_t nego_mtu_pado, nego_mtu_pads;
int pppfd, client_state;
static int getpackets(int, u_int8_t *, u_int8_t *, struct ether_addr *,
@@ -80,57 +82,79 @@ int
client_mode(int bfd, u_int8_t *sysname, u_int8_t *srvname, struct
ether_addr *myea)
{
struct ether_addr rmea;
- fd_set *fdsp = NULL;
- int r = 0, max, oldmax = 0;
+ int kq = -1;
+ int r = 0;
+ int nchanges = 0;
+ struct kevent eventlist[2];
+ int pppfd_set = 0;
+ int bfd_idx = -1;
+ int pppfd_idx = -1;
+ int i = 0;
+ kq = kqueue();
+ if (kq < 0)
+ return -1;
+
+ EV_SET(eventlist + nchanges, bfd, EVFILT_READ, EV_ADD, 0, 0, NULL);
+ ++nchanges;
+
pppfd = -1;
client_sessionid = 0xffff;
client_state = -1;
+ nego_mtu_pado = nego_mtu_pads = PPPOE_MTU;
r = send_padi(bfd, myea, srvname);
- if (r <= 0)
+ if (r <= 0) {
+ close(kq);
return (r);
+ }
for (;;) {
- max = bfd;
- if (pppfd >= 0 && pppfd >= max)
- max = pppfd;
- max++;
-
- if (max > oldmax) {
- if (fdsp != NULL)
- free(fdsp);
- fdsp = (fd_set *)calloc(howmany(max, NFDBITS),
- sizeof(fd_mask));
- if (fdsp == NULL) {
- r = -1;
- break;
+ if (pppfd < 0) {
+ pppfd_set = 0;
+ } else {
+ if (!pppfd_set) {
+ EV_SET(eventlist + nchanges, pppfd,
EVFILT_READ, EV_ADD, 0, 0, NULL);
+ ++nchanges;
+ pppfd_set = 1;
}
- oldmax = max;
}
- bzero(fdsp, howmany(max, NFDBITS) * sizeof(fd_mask));
- if (pppfd >= 0)
- FD_SET(pppfd, fdsp);
- FD_SET(bfd, fdsp);
+ r = kevent(kq, eventlist, nchanges, eventlist, 2, NULL);
+ nchanges = 0;
+ if (r <= 0) {
+ if (timer_hit())
+ timer_clr();
+ break;
+ }
- r = select(max, fdsp, NULL, NULL, NULL);
- if (r < 0) {
- if (errno == EINTR) {
- if (timer_hit())
- timer_clr();
- break;
+ bfd_idx = -1;
+ pppfd_idx = -1;
+ for (i = 0; i < r; ++i) {
+ if (eventlist[i].ident == bfd) {
+ bfd_idx = i;
+ } else if (eventlist[i].ident == pppfd) {
+ pppfd_idx = i;
}
- break;
}
- if (FD_ISSET(bfd, fdsp)) {
- r = getpackets(bfd, srvname, sysname, myea, &rmea);
- if (r <= 0)
+
+ if (bfd_idx >= 0) {
+ u_short flags = eventlist[bfd_idx].flags;
+ if (flags & EV_EOF || flags & EV_ERROR) {
+ r = -1;
break;
+ }
+ r = getpackets(bfd, srvname, sysname, myea, &rmea);
+ if (r <= 0)
+ break;
}
- if (pppfd >= 0 && FD_ISSET(pppfd, fdsp)) {
- r = ppp_to_bpf(bfd, pppfd, myea, &rmea,
- client_sessionid);
+ if (pppfd >= 0 && pppfd_idx >= 0) {
+ u_short flags = eventlist[pppfd_idx].flags;
+ if (flags & EV_EOF || flags & EV_ERROR) {
+ r = -1;
+ break;
+ }
+ r = ppp_to_bpf(bfd, pppfd, myea, &rmea,
client_sessionid);
if (r < 0)
break;
}
@@ -141,22 +165,21 @@ client_mode(int bfd, u_int8_t *sysname, u_int8_t *srvn
pppfd = -1;
}
- if (fdsp != NULL)
- free(fdsp);
-
+ close(kq);
return (r);
}
static int
send_padi(int fd, struct ether_addr *ea, u_int8_t *srv)
{
- struct iovec iov[10];
+ struct iovec iov[12];
struct pppoe_header ph = {
PPPOE_VERTYPE(1, 1),
PPPOE_CODE_PADI, 0, 0
};
- struct pppoe_tag thost, tserv;
+ struct pppoe_tag thost, tserv, tmaxpayload;
u_int16_t etype = htons(ETHERTYPE_PPPOEDISC);
+ u_int16_t maxpayload;
int i = 0;
/* ether_header */
@@ -196,6 +219,19 @@ send_padi(int fd, struct ether_addr *ea, u_int8_t *srv
}
tserv.len = htons(tserv.len);
+ /* ppp-max-payload tag (optional) */
+ if (rfc_4638_mtu > PPPOE_MTU) {
+ tmaxpayload.type = htons(PPPOE_TAG_PPP_MAX_PAYLOAD);
+ tmaxpayload.len = htons(sizeof(maxpayload));
+ tmaxpayload.val = (u_int8_t*) &maxpayload;
+ maxpayload = htons(rfc_4638_mtu);
+ iov[i].iov_base = &tmaxpayload;
+ iov[i++].iov_len = sizeof(tmaxpayload.len) +
sizeof(tmaxpayload.type);
+ iov[i].iov_base = &maxpayload;
+ iov[i++].iov_len = sizeof(maxpayload);
+ ph.len += sizeof(tmaxpayload.len) + sizeof(tmaxpayload.type) +
sizeof(maxpayload);
+ }
+
ph.len = htons(ph.len);
client_state = STATE_EXPECT_PADO;
@@ -208,9 +244,10 @@ send_padr(int bfd, u_int8_t *srv, struct ether_addr *m
struct ether_addr *rmea, struct ether_header *eh,
struct pppoe_header *ph, struct tag_list *tl)
{
- struct iovec iov[12];
+ struct iovec iov[14];
u_int16_t etype = htons(ETHERTYPE_PPPOEDISC);
- struct pppoe_tag hutag, svtag;
+ struct pppoe_tag hutag, svtag, mptag;
+ u_int16_t maxpayload = htons(nego_mtu_pado);
struct tag_node *n;
int idx = 0, slen;
@@ -251,6 +288,17 @@ send_padr(int bfd, u_int8_t *srv, struct ether_addr *m
iov[idx++].iov_len = slen;
}
+ /* PPP-Max-Payload */
+ if (nego_mtu_pado > PPPOE_MTU) {
+ mptag.type = htons(PPPOE_TAG_PPP_MAX_PAYLOAD);
+ mptag.len = htons(sizeof(maxpayload));
+ iov[idx].iov_base = &mptag;
+ iov[idx++].iov_len = sizeof(mptag.type) + sizeof(mptag.len);
+ iov[idx].iov_base = &maxpayload;
+ iov[idx++].iov_len = sizeof(maxpayload);
+ ph->len += sizeof(mptag.type) + sizeof(mptag.len) +
sizeof(maxpayload);
+ }
+
n = tag_lookup(tl, PPPOE_TAG_RELAY_SESSION, 0);
if (n != NULL) {
iov[idx].iov_base = &n->type;
@@ -369,8 +417,7 @@ getpackets(int bfd, u_int8_t *srv, u_int8_t *sysname,
if ((r = bpf_to_ppp(pppfd, len, mpkt)) < 0)
return (-1);
if (r == 0) {
- usleep(100000);
- continue;
+ goto next;
}
}
next:
@@ -413,6 +460,16 @@ recv_pado(int bfd, u_int8_t *srv, struct ether_addr *m
if (bcmp(n->val, &client_cookie, sizeof(client_cookie)))
goto out;
+ if (rfc_4638_mtu > PPPOE_MTU) {
+ n = tag_lookup(&tl, PPPOE_TAG_PPP_MAX_PAYLOAD, 0);
+ if (n != NULL) {
+ nego_mtu_pado = ntohs(*((u_int16_t*)n->val));
+ if (nego_mtu_pado < PPPOE_MTU) {
+ nego_mtu_pado = PPPOE_MTU;
+ }
+ }
+ }
+
r = 0;
slen = (srv == NULL) ? 0 : strlen((char *)srv);
while ((n = tag_lookup(&tl, PPPOE_TAG_SERVICE_NAME, r)) != NULL) {
@@ -468,6 +525,15 @@ recv_pads(int bfd, u_int8_t *srv, u_int8_t *sysname,
goto out;
if (bcmp(n->val, &client_cookie, sizeof(client_cookie)))
goto out;
+
+ n = tag_lookup(&tl, PPPOE_TAG_PPP_MAX_PAYLOAD, 0);
+ if (n != NULL && n->len == sizeof(nego_mtu_pads)) {
+ nego_mtu_pads = ntohs(*((u_int16_t*)n->val));
+ if (nego_mtu_pads < PPPOE_MTU ||
+ nego_mtu_pads > rfc_4638_mtu) {
+ nego_mtu_pads = PPPOE_MTU;
+ }
+ }
if (ph->sessionid == 0) {
timer_clr();
diff -upr pppoe/common.c pppoe.kqueue/common.c
--- pppoe/common.c Thu May 6 21:29:04 2004
+++ pppoe.kqueue/common.c Sun Mar 27 20:42:03 2011
@@ -143,7 +143,7 @@ ppp_to_bpf(int bfd, int pppfd, struct ether_addr *myea
int r;
if (pktbuf == NULL) {
- pktbuf = (u_int8_t *)malloc(PPPOE_MTU);
+ pktbuf = (u_int8_t *)malloc(rfc_4638_mtu);
if (pktbuf == NULL)
return (-1);
}
@@ -151,7 +151,7 @@ ppp_to_bpf(int bfd, int pppfd, struct ether_addr *myea
iov[0].iov_base = trash;
iov[0].iov_len = 2;
iov[1].iov_base = pktbuf;
- iov[1].iov_len = PPPOE_MTU;
+ iov[1].iov_len = rfc_4638_mtu;
r = readv(pppfd, iov, 2);
if (r <= 0)
return (-1);
diff -upr pppoe/pppoe.8 pppoe.kqueue/pppoe.8
--- pppoe/pppoe.8 Sat Nov 17 14:45:32 2007
+++ pppoe.kqueue/pppoe.8 Mon Mar 28 19:49:11 2011
@@ -25,7 +25,7 @@
.\" POSSIBILITY OF SUCH DAMAGE.
.\"
.\"
-.Dd $Mdocdate: November 17 2007 $
+.Dd $Mdocdate: March 27 2011 $
.Dt PPPOE 8
.Os
.Sh NAME
@@ -37,6 +37,7 @@
.Op Fl i Ar interface
.Op Fl n Ar service
.Op Fl p Ar system
+.Op Fl m Ar mtu
.Sh DESCRIPTION
The
.Nm pppoe
@@ -67,6 +68,14 @@ This argument is passed, uninterpreted, to
It can be used to specify the configuration data to be used for
PPP Over Ethernet connections.
This option is only used in server mode.
+.It Fl m Ar mtu
+Attempt to negotiate
+a higher mtu, using RFC 4638. It is your responsibility to ensure that the
+ethernet
+.Ar interface
+works and is configured with an mtu at least 8 bytes
+greater than this value.
+This option is only used in client mode.
.It Fl s
If this option is specified,
.Nm
@@ -223,6 +232,11 @@ to adjust the maximum segment size on outgoing SYN pac
.%T A Method for Transmitting PPP Over Ethernet (PPPoE)
.%A L. Mamakos, et al.
.Re
+.Rs
+.%R RFC 4638
+.%T Accommodating a Maximum Transit Unit/Maximum Receive Unit
(MTU/MRU) Greater Than 1492 in the Point-to-Point Protocol over
Ethernet (PPPoE)
+.%A P. Arberg, et al.
+.Re
.Sh HISTORY
This implementation of
.Nm pppoe
@@ -244,4 +258,7 @@ As such it will have much more overhead than a kernel
.Pp
The
.Ar service
+is not currently used by the server code.
+The
+.Ar mtu
is not currently used by the server code.
diff -upr pppoe/pppoe.c pppoe.kqueue/pppoe.c
--- pppoe/pppoe.c Tue Jun 24 09:21:46 2008
+++ pppoe.kqueue/pppoe.c Sun Mar 27 20:43:30 2011
@@ -52,6 +52,7 @@
#include "pppoe.h"
+u_int16_t rfc_4638_mtu = PPPOE_MTU;
int option_verbose = 0;
u_char etherbroadcastaddr[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
@@ -78,7 +79,7 @@ main(int argc, char **argv)
if ((pw = getpwnam("_ppp")) == NULL)
err(EX_CONFIG, "getpwnam(\"_ppp\")");
- while ((c = getopt(argc, argv, "svi:n:p:")) != -1) {
+ while ((c = getopt(argc, argv, "svm:i:n:p:")) != -1) {
switch (c) {
case 'i':
if (ifname != NULL) {
@@ -111,6 +112,11 @@ main(int argc, char **argv)
case 'v':
option_verbose++;
break;
+ case 'm':
+ rfc_4638_mtu = (u_int16_t) atoi(optarg);
+ if (rfc_4638_mtu < PPPOE_MTU)
+ rfc_4638_mtu = PPPOE_MTU;
+ break;
default:
usage();
return (EX_USAGE);
@@ -125,6 +131,10 @@ main(int argc, char **argv)
if (getifhwaddr(ifname, ifnambuf, &ea) < 0)
return (EX_IOERR);
+
+ if (smode) {
+ rfc_4638_mtu = PPPOE_MTU;
+ }
bpffd = setupfilter(ifnambuf, &ea, smode);
if (bpffd < 0)
diff -upr pppoe/pppoe.h pppoe.kqueue/pppoe.h
--- pppoe/pppoe.h Thu May 6 21:29:04 2004
+++ pppoe.kqueue/pppoe.h Mon Mar 28 19:00:30 2011
@@ -76,10 +76,12 @@ struct pppoe_tag {
#define PPPOE_TAG_AC_COOKIE 0x0104 /* Access Concentratr
Cookie */
#define PPPOE_TAG_VENDOR_SPEC 0x0105 /* Vendor Specific */
#define PPPOE_TAG_RELAY_SESSION 0x0110 /* Relay Session Id */
+#define PPPOE_TAG_PPP_MAX_PAYLOAD 0x0120 /* RFC 4638 PPP-Max-Payload */
#define PPPOE_TAG_SERVICE_NAME_ERROR 0x0201 /* Service Name Error */
#define PPPOE_TAG_AC_SYSTEM_ERROR 0x0202 /* Acc. Concentrator
Error */
#define PPPOE_TAG_GENERIC_ERROR 0x0203 /* Generic Error */
+extern u_int16_t rfc_4638_mtu;
extern int option_verbose;
extern u_char etherbroadcastaddr[];
