[syslinux] [PATCH][GIT-PULL] lwIP undiif: Fixes for VMware platforms and general fixes
Eric W. Biederman
ebiederm at xmission.com
Fri Oct 21 20:15:13 PDT 2011
Gene Cumm <gene.cumm at gmail.com> writes:
> On Fri, Oct 21, 2011 at 06:15, Eric W. Biederman <ebiederm at xmission.com> wrote:
>> 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.
>
> By this I mean that it would be up to the UNDI and undiif to address
> ARP as the UNDI might not be Ethernet in the first place. This would
> accelerate the logic path decisions but won't change the need to let
> the sender finish up the packet before the receiver can demux
> properly.
>
>> 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
>
> Aside from the lack of core/lwip/src/include/netif/undiarp.h and a
> minor patch issue in the tail (spacing), it can apply. This does
> appear to function. So tcpip_thread() only uses undi_input() on
> non-Ethernet but I also don't see NETIF_FLAG_UNDIARP being set on
> flags (I'd presume to be implemented in the future).
Good. I'm glad that it functions. Thanks for testing.
My apologies for the slightly damaged patch. It appears I had failed to
regenerate the patch after adding undiarp.h and setting
NETIF_FLAG_UNDIARP.
As for the spacing issues I am probably working against and older
version of the code base.
>From dd12a3bf9230f6db1465b000d477ca32067f1017 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/include/netif/undiarp.h | 20 ++++++++++++++
core/lwip/src/netif/undiif.c | 47 +++++++++++++++++++++++++++-----
4 files changed, 68 insertions(+), 8 deletions(-)
create mode 100644 core/lwip/src/include/netif/undiarp.h
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/include/netif/undiarp.h b/core/lwip/src/include/netif/undiarp.h
new file mode 100644
index 0000000..7cdec6c
--- /dev/null
+++ b/core/lwip/src/include/netif/undiarp.h
@@ -0,0 +1,20 @@
+#ifndef __NETIF_UNDIARP_H__
+#define __NETIF_UNDIARP_H__
+
+#include "lwip/opt.h"
+
+#if LWIP_ARP /* don't build if not configured for use in lwipopts.h */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+err_t undi_input(struct pbuf *p, struct netif *netif);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* LWIP_ARP */
+
+#endif /* __NETIF_ARP_H__ */
diff --git a/core/lwip/src/netif/undiif.c b/core/lwip/src/netif/undiif.c
index f21eb0b..6a7c5ae 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"
@@ -270,6 +271,8 @@ low_level_init(struct netif *netif)
/* don't set NETIF_FLAG_ETHARP if this device is not an ethernet one */
if (undi_is_ethernet(netif))
netif->flags |= NETIF_FLAG_ETHARP;
+ else
+ netif->flags |= NETIF_FLAG_UNDIARP;
/* Install the interrupt vector */
pxe_irq_vector = undi_info.IntNumber;
@@ -1232,6 +1235,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 +1299,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 +1319,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 +1401,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