[syslinux] [PATCH][GIT-PULL] lwIP undiif: Fixes for VMware platforms and general fixes
Eric W. Biederman
ebiederm at xmission.com
Fri Oct 21 03:15:38 PDT 2011
Gene Cumm <gene.cumm at gmail.com> writes:
> On Thu, Oct 20, 2011 at 21:11, Eric W. Biederman <ebiederm at xmission.com> wrote:
>> tcpip_input always calls ethernet_input if you are an ethernet device.
>> So the only practical difference is bounding through an extra thread.
>>
>> Maybe there is a good reason for bouncing through an extra thread.
>> I seem to have vague memories about it just being plain less efficient
>> and killing through put.
>
> A wider window helps to a point.
>
>> I did not use tcpip_input deliberately. At the very least because it
>> was an apparent unnecessary complication.
>>
>> I guess I would tend to focus on why are we not ready to receive
>> a packet? That sounds like a more straight forward bug to fix than
>> weird crazy timing issues.
>
> tcp_output() eventually leads to undiif_output() but before returning
> all the way, the packet is receieved on undiif_input(). If we attempt
> at this instant, there is no open socket. If we attempt once
> tcp_output() finishes, the PCB is finalized and the socket is valid.
> I have some lengthy debug output of failures and successes if you'd
> like (including extra statements I put in and have prepped in a branch
> purely for debugging that I can publish if you wish to see it)
I think looking at the traces would be useful. There is something else
interesting in all of this. To run the interrupt handler we already
have to switch threads. I think that is what ultimately led me to
thinking calling tcpip_input was just plain silly.
I forget the task switching model but perhaps it would be worth
playing with a voluntary preemption point. Or maybe what we need
instead fewer threads.
Doh! What tcpip_input gets us is that we run everything single threaded.
Let me just say that lwip has the most convoluted idea of a bit network
stack lock that I have ever seen. Posting messages in a multi threaded
context so a single thread can do all of the work.
So I guess at that level using tcpip_input makes a lot of sense. Less
multitasking so there are fewer surprises.
>> To make things particularly clean and beautiful the arp layer needs to
>> be abstracted away from ethernet. But that hasn't happened yet.
>
> This has me leaning towards not setting that flag so that lwIP doesn't
> think it can do ARP itself.
I don't follow. We always need to do ARP. The UNDI layer doesn't know
how. So lwip in some form will always need to do arp.
question is do we generate the link layer header ourselves.
After having realized that the point of the tcpip_thread is to
single thread the tcpip stack. I have cooked up the following
untested patch. But I think it solves our issues cleanly.
Does that patch fix your issues?
Eric
>From 026dc54baac33f22819e6c36f0b9b2de8e9f5ecf Mon Sep 17 00:00:00 2001
From: Eric W. Biederman <ebiederm at xmission.com>
Date: Fri, 21 Oct 2011 03:03:53 -0700
Subject: [PATCH] undi: Process all of packets on the tcpip thread.
It turns out that in practice passing everything over to the
tcpip_thread acts as a big networking stack lock for lwip.
It seems silly me to implement threads just so you can disable
them by running everything single threaded but that looks like
the way it needs to be for lwip.
Signed-off-by: Eric W. Biederman <ebiederm at xmission.com>
---
core/lwip/src/api/tcpip.c | 6 ++++
core/lwip/src/include/lwip/netif.h | 3 ++
core/lwip/src/netif/undiif.c | 45 +++++++++++++++++++++++++++++------
3 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/core/lwip/src/api/tcpip.c b/core/lwip/src/api/tcpip.c
index 9c0ba5b..e265725 100644
--- a/core/lwip/src/api/tcpip.c
+++ b/core/lwip/src/api/tcpip.c
@@ -52,6 +52,7 @@
#include "lwip/tcpip.h"
#include "lwip/init.h"
#include "netif/etharp.h"
+#include "netif/undiarp.h"
#include "netif/ppp_oe.h"
/* global variables */
@@ -278,6 +279,11 @@ tcpip_thread(void *arg)
ethernet_input(msg->msg.inp.p, msg->msg.inp.netif);
} else
#endif /* LWIP_ARP */
+#if LWIP_ARP
+ if (msg->msg.inp.netif->flags & NETIF_FLAG_UNDIARP) {
+ undi_input(msg->msg.inp.p, msg->msg.inp.netif);
+ } else
+#endif /* LWIP_ARP */
{ ip_input(msg->msg.inp.p, msg->msg.inp.netif);
}
memp_free(MEMP_TCPIP_MSG_INPKT, msg);
diff --git a/core/lwip/src/include/lwip/netif.h b/core/lwip/src/include/lwip/netif.h
index 462d349..a3adc99 100644
--- a/core/lwip/src/include/lwip/netif.h
+++ b/core/lwip/src/include/lwip/netif.h
@@ -80,6 +80,9 @@ extern "C" {
#define NETIF_FLAG_ETHARP 0x20U
/** if set, the netif has IGMP capability */
#define NETIF_FLAG_IGMP 0x40U
+/** if set, the netif is an device using ARP */
+#define NETIF_FLAG_UNDIARP 0x80U
+
/** Generic data structure used for all lwIP network interfaces.
* The following fields should be filled in by the initialization
diff --git a/core/lwip/src/netif/undiif.c b/core/lwip/src/netif/undiif.c
index f21eb0b..38005be 100644
--- a/core/lwip/src/netif/undiif.c
+++ b/core/lwip/src/netif/undiif.c
@@ -56,6 +56,7 @@
#include <lwip/stats.h>
#include <lwip/snmp.h>
#include "netif/etharp.h"
+#include "netif/undiarp.h"
#include "netif/ppp_oe.h"
#include "lwip/netifapi.h"
#include "lwip/tcpip.h"
@@ -1232,6 +1233,33 @@ undiarp_input(struct netif *netif, struct pbuf *p)
}
/**
+ * Process received undi frames. Using this function instead of directly
+ * calling ip_input and passing ARP frames through undiarp in ethernetif_input,
+ * the ARP cache is protected from concurrent access.
+ *
+ * @param p the recevied packet, p->payload pointing to the undi header
+ * @param netif the network interface on which the packet was received
+ */
+err_t
+undi_input(struct pbuf *p, struct netif *netif)
+{
+ u8_t undi_prot = p->flags;
+ switch (undi_prot) {
+ case P_IP:
+ /* Pass to IP layer */
+ ip_input(p, netif);
+ break;
+ case P_ARP:
+ undiarp_input(netif, p);
+ break;
+ }
+
+ /* This means the pbuf is freed or consumed,
+ so the caller doesn't have to free it again */
+ return ERR_OK;
+}
+
+/**
* This function should be called when a packet is ready to be read
* from the interface. It uses the function low_level_input() that
* should handle the actual reception of bytes from the network
@@ -1269,7 +1297,7 @@ void undiif_input(t_PXENV_UNDI_ISR *isr)
case ETHTYPE_PPPOE:
#endif /* PPPOE_SUPPORT */
/* full packet send to tcpip_thread to process */
- if (ethernet_input(p, &undi_netif)!=ERR_OK)
+ if (undi_netif.input(p, &undi_netif)!=ERR_OK)
{ LWIP_DEBUGF(NETIF_DEBUG, ("undiif_input: IP input error\n"));
pbuf_free(p);
p = NULL;
@@ -1289,13 +1317,14 @@ void undiif_input(t_PXENV_UNDI_ISR *isr)
} else {
switch(undi_prot) {
case P_IP:
- /* pass to IP layer */
- ip_input(p, &undi_netif);
- break;
-
case P_ARP:
- /* pass p to ARP module */
- undiarp_input(&undi_netif, p);
+ /* Hack store the flags in the unused pbuf flags field */
+ p->flags = undi_prot;
+ if (undi_netif.input(p, &undi_netif)!=ERR_OK)
+ { LWIP_DEBUGF(NETIF_DEBUG, ("undiif_input: IP input error\n"));
+ pbuf_free(p);
+ p = NULL;
+ }
break;
default:
@@ -1370,7 +1399,7 @@ int undiif_start(uint32_t ip, uint32_t netmask, uint32_t gw)
((uint8_t *)gw)[3]);
err = netifapi_netif_add(&undi_netif,
(struct ip_addr *)&ip, (struct ip_addr *)&netmask, (struct ip_addr *)gw,
- NULL, undiif_init, ip_input);
+ NULL, undiif_init, tcpip_input);
if (err)
return err;
--
1.7.2.5
More information about the Syslinux
mailing list