Add (G)ARP sender to fix switch broadcast storms #413

markwebster · 2017-12-19T05:16:48Z

On many switches it's basically impossible to use pkt-gen without sending regular ARPs to maintain association with the switch port. When the PHY resets into netmap mode, the association is usually lost, causing traffic to be broadcast to all ports. This breaks the receiver and destroys network performance.

Also added link state auto wait option when you specify -w0.

I have tested on Linux boxes only, so there is a chance of broken builds for other platforms. Could someone check FreeBSD etc?

Needed to maintain {mac:ip} assocation on the switch port. This likely fixes many of the reported pkt-gen issues. Also added PHY link state poll ("auto wait") when '-w0' is specified.

vmaffione · 2017-12-21T11:52:47Z

Yes, this breaks compilation on FreeBSD

markwebster · 2017-12-21T16:20:24Z

Can you paste the build error please?

vmaffione · 2017-12-22T08:46:21Z

cc -O2 -pipe -Werror -Wall -Wunused-function -I ../../sys -I ../../apps/include -Wextra -DNO_PCAP    pkt-gen.c  -lpthread -lm -o pkt-gen
pkt-gen.c:216:20: error: field has incomplete type 'struct ether_arp'
                struct ether_arp arp4;
                                 ^
pkt-gen.c:216:10: note: forward declaration of 'struct ether_arp'
                struct ether_arp arp4;
                       ^
pkt-gen.c:687:34: error: use of undeclared identifier 'PF_PACKET'
                if (!sdl || sdl->sdl_family != PF_PACKET)
                                               ^
pkt-gen.c:804:20: error: no member named 'ipv4' in 'struct pkt'
        memcpy(&ip, &pkt->ipv4.ip, sizeof(ip));
                     ~~~  ^
pkt-gen.c:805:21: error: no member named 'ipv4' in 'struct pkt'
        memcpy(&udp, &pkt->ipv4.udp, sizeof(udp));
                      ~~~  ^
pkt-gen.c:885:15: error: no member named 'ipv4' in 'struct pkt'
        memcpy(&pkt->ipv4.ip, &ip, sizeof(ip));
                ~~~  ^
pkt-gen.c:886:15: error: no member named 'ipv4' in 'struct pkt'
        memcpy(&pkt->ipv4.udp, &udp, sizeof(udp));
                ~~~  ^
pkt-gen.c:902:21: error: no member named 'ipv6' in 'struct pkt'
        memcpy(&ip6, &pkt->ipv6.ip, sizeof(ip6));
                      ~~~  ^
pkt-gen.c:903:21: error: no member named 'ipv6' in 'struct pkt'
        memcpy(&udp, &pkt->ipv6.udp, sizeof(udp));
                      ~~~  ^
pkt-gen.c:972:15: error: no member named 'ipv6' in 'struct pkt'
        memcpy(&pkt->ipv6.ip, &ip6, sizeof(ip6));
                ~~~  ^
pkt-gen.c:973:15: error: no member named 'ipv6' in 'struct pkt'
        memcpy(&pkt->ipv6.udp, &udp, sizeof(udp));
                ~~~  ^
pkt-gen.c:1035:18: error: no member named 'ipv4' in 'struct pkt'
                bcopy(payload, PKT(pkt, body, targ->g->af) + i, l0);
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
pkt-gen.c:221:29: note: expanded from macro 'PKT'
    ((af) == AF_INET ? (p)->ipv4.f: (p)->ipv6.f)
                       ~~~  ^
pkt-gen.c:1035:18: error: no member named 'ipv6' in 'struct pkt'
                bcopy(payload, PKT(pkt, body, targ->g->af) + i, l0);
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
pkt-gen.c:221:42: note: expanded from macro 'PKT'
    ((af) == AF_INET ? (p)->ipv4.f: (p)->ipv6.f)
                                    ~~~  ^
pkt-gen.c:1037:2: error: no member named 'ipv4' in 'struct pkt'
        PKT(pkt, body, targ->g->af)[i - 1] = '\0';
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
pkt-gen.c:221:29: note: expanded from macro 'PKT'
    ((af) == AF_INET ? (p)->ipv4.f: (p)->ipv6.f)
                       ~~~  ^
pkt-gen.c:1037:2: error: no member named 'ipv6' in 'struct pkt'
        PKT(pkt, body, targ->g->af)[i - 1] = '\0';
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
pkt-gen.c:221:42: note: expanded from macro 'PKT'
    ((af) == AF_INET ? (p)->ipv4.f: (p)->ipv6.f)
                                    ~~~  ^
pkt-gen.c:1046:21: error: no member named 'ipv4' in 'struct pkt'
                memcpy(&ip, &pkt->ipv4.ip, sizeof(ip));
                             ~~~  ^
pkt-gen.c:1047:19: error: no member named 'ipv4' in 'struct pkt'
                udp_ptr = &pkt->ipv4.udp;
                           ~~~  ^
pkt-gen.c:1060:16: error: no member named 'ipv4' in 'struct pkt'
                memcpy(&pkt->ipv4.ip, &ip, sizeof(ip));
                        ~~~  ^
pkt-gen.c:1063:22: error: no member named 'ipv4' in 'struct pkt'
                memcpy(&ip6, &pkt->ipv4.ip, sizeof(ip6));
                              ~~~  ^
pkt-gen.c:1064:19: error: no member named 'ipv6' in 'struct pkt'
                udp_ptr = &pkt->ipv6.udp;
                           ~~~  ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]

markwebster · 2017-12-22T09:13:01Z

Try now?

vmaffione · 2017-12-22T09:42:28Z

pkt-gen.c:1271:33: error: use of undeclared identifier 'ETH_ALEN'
                memset(eh->ether_dhost, 0xff, ETH_ALEN);

FYI, here there are some ready to use FreeBSD VM images for many hypervisors
https://download.freebsd.org/ftp/releases/VM-IMAGES/11.1-RELEASE/amd64/Latest/
this could speed up your tests.

markwebster · 2017-12-22T09:46:19Z

Cool, I'll do that next time. It should be fixed now

giuseppelettieri · 2017-12-22T10:16:14Z

I would merge the contribution now, then fix the hardcoded 6 later.

vmaffione · 2017-12-22T10:47:59Z

apps/pkt-gen/pkt-gen.c

+		int copy = options & OPT_COPY || slot->flags & NS_BUF_CHANGED;
+		/* sender_body() drops OPT_COPY after starting, but we need to
+		 * copy over any ARP packets lingering in the txring */
+		copy |= (old->eh.ether_type == htons(ETHERTYPE_ARP));


This is likely a cache miss, I would do that only if -G is specified

It's actually worse than it looks... Only thread 0 can have (targ->garp != 0) but here all TX threads are checking for no reason. Let me improve this.

(My initial implementation used the "sem[]" area in the netmap_ring to store a dirty flag, for when the ring needs to be checked. It works fine but it's fiddly and I don't think people would like it.)

vmaffione · 2017-12-22T10:51:01Z

apps/pkt-gen/pkt-gen.c

@@ -1570,6 +1690,13 @@ sender_body(void *data)
 			nexttime = timespec_add(nexttime, targ->g->tx_period);
 			wait_time(nexttime);
 		}
+		if (targ->garp) {
+			clock_gettime(CLOCK_REALTIME_PRECISE, &tmptime);


Do we really need to be CLOCK_REALTIME_PRECISE? COARSE will be enough, and it's faster.

Oh, the Linux manpage says COARSE is Linux-specific. I'm sure not what other supported platforms will need a work-around. How about gettimeofday?

vmaffione · 2017-12-22T10:51:24Z

apps/pkt-gen/pkt-gen.c

@@ -1792,6 +1925,14 @@ receiver_body(void *data)
 			goto quit;
 		}
 #endif /* !BUSYWAIT */
+		if (targ->garp) {
+			clock_gettime(CLOCK_REALTIME_PRECISE, &tmptime);


Same as above

I'm just going along with what was already being used.

Sorry, I had not seen that the _PRECISE is just a macro that maps back to CLOCK_REALTIME, which is ok.

Aha, you just beat me to saying that

vmaffione · 2017-12-22T10:53:38Z

apps/pkt-gen/pkt-gen.c

+		D("Ready...");
+	} else {
+		D("Wait for phy reset");
+		for (i = 5*4; i > 0; i--) {


You are waiting for 5 seconds here (max)

D("Wait up to 5 seconds for phy reset")

The check is now only done on thread 0, and before touching the slot buffer we check if slot->len == sizeof(ARP packet), ie 42, whereas the smallest packet size pkt-gen does without crashing is 50. (This commit also includes a better minimum packet size constraint for the -l argument). Some related code has been tidied up, and I've added #defines for IPv4 and MAC address sizes (IP4_ALEN, ETH_ALEN) to get rid of some magic numbers.

markwebster · 2017-12-31T11:40:29Z

Sorry about the delay. I've updated this now!

vmaffione · 2018-01-12T08:10:42Z

apps/pkt-gen/pkt-gen.c

-	paylen = targ->g->pkt_size - sizeof(*eh) -
-	    (targ->g->af == AF_INET ? sizeof(ip): sizeof(ip6));
+	paylen = targ->g->pkt_size -
+	    PKT_OVERHD(targ->g->af, targ->g->virt_header);


pkt_size does not include virt_header... why are you including that in PKT_OVERHD?

vmaffione · 2018-01-12T08:18:16Z

apps/pkt-gen/pkt-gen.c

+			bcopy(src_mac, pkt->arp4.arp_sha, ETH_ALEN);
+			bcopy(src_mac, pkt->arp4.arp_tha, ETH_ALEN);
+			bcopy(&src_ip, pkt->arp4.arp_spa, IP4_ALEN);
+			bcopy(&src_ip, pkt->arp4.arp_spa, IP4_ALEN);


The last two lines are the same... did you mean to use dst_ip in the second one?

vmaffione · 2018-01-12T08:19:18Z

apps/pkt-gen/pkt-gen.c

+			bzero(pkt->arp4.arp_tha, ETH_ALEN);
+			bcopy(&src_ip, pkt->arp4.arp_spa, IP4_ALEN);
+			bcopy(&dst_ip, pkt->arp4.arp_tpa, IP4_ALEN);
+		}


please move the lines that are common to both if and else out of the if-else construct

vmaffione · 2018-01-12T08:34:08Z

apps/pkt-gen/pkt-gen.c

+
+		/* sender_body() drops OPT_COPY after starting, but we need
+		 * to copy over any ARP packets lingering in the txring */
+		if (!copy && slot->len == arp_size) {


Could you replace this condition with options & OPT_GARP? In this way you make only a single check in the common case.

vmaffione · 2018-01-22T13:54:58Z

any news on this?

markwebster · 2018-01-25T09:10:53Z

I'm travelling for the next 4 weeks. If I get a chance to do any coding, I'll follow this up. Sorry!

markwebster added 3 commits December 19, 2017 04:40

pkt-gen: -G for (G)ARP to prevent switch broadcast storms

b705f7b

Needed to maintain {mac:ip} assocation on the switch port. This likely fixes many of the reported pkt-gen issues. Also added PHY link state poll ("auto wait") when '-w0' is specified.

Removed a #define left over from an experiment

2bae22b

Fixed another typo, last commit was from wrong dir

16f4bb1

markwebster added 2 commits December 22, 2017 09:00

pkt-gen: fix FreeBSD build

b40972c

pkt-gen: more fix FreeBSD build

06832d3

pkt-gen: one more FreeBSD fix

7dc6dc0

vmaffione reviewed Dec 22, 2017

View reviewed changes

markwebster added 2 commits December 31, 2017 00:29

Merge remote-tracking branch 'upstream/master'

451452e

vmaffione reviewed Jan 12, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add (G)ARP sender to fix switch broadcast storms #413

Add (G)ARP sender to fix switch broadcast storms #413

markwebster commented Dec 19, 2017

vmaffione commented Dec 21, 2017

markwebster commented Dec 21, 2017

vmaffione commented Dec 22, 2017

markwebster commented Dec 22, 2017

vmaffione commented Dec 22, 2017

markwebster commented Dec 22, 2017

giuseppelettieri commented Dec 22, 2017

vmaffione Dec 22, 2017

markwebster Dec 22, 2017

vmaffione Dec 22, 2017

markwebster Dec 22, 2017

vmaffione Dec 22, 2017

markwebster Dec 22, 2017

vmaffione Dec 22, 2017

markwebster Dec 22, 2017

vmaffione Dec 22, 2017

markwebster Dec 22, 2017

vmaffione Dec 22, 2017

markwebster commented Dec 31, 2017

vmaffione Jan 12, 2018

vmaffione Jan 12, 2018 •

edited

vmaffione Jan 12, 2018

vmaffione Jan 12, 2018

vmaffione commented Jan 22, 2018

markwebster commented Jan 25, 2018

Add (G)ARP sender to fix switch broadcast storms #413

Are you sure you want to change the base?

Add (G)ARP sender to fix switch broadcast storms #413

Conversation

markwebster commented Dec 19, 2017

vmaffione commented Dec 21, 2017

markwebster commented Dec 21, 2017

vmaffione commented Dec 22, 2017

markwebster commented Dec 22, 2017

vmaffione commented Dec 22, 2017

markwebster commented Dec 22, 2017

giuseppelettieri commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwebster commented Dec 31, 2017

Choose a reason for hiding this comment

vmaffione Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmaffione commented Jan 22, 2018

markwebster commented Jan 25, 2018

vmaffione Jan 12, 2018 •

edited