gnrc_netif_ieee802154: drop duplicate broadcast packets (optionally) by miri64 · Pull Request #7577 · RIOT-OS/RIOT

miri64 · 2017-09-06T15:20:58Z

Can be activated with USEMODULE += gnrc_netif_dedup (considering gnrc_netif is activated, see #7424). Testing procedures see #7577 (comment)

Depends on ~~#7409~~ (merged).

jnohlgard · 2017-09-06T16:00:35Z

Why only broadcast packets? The dedup is just as useful for unicast, for example when a packet is correctly received, but the ack is not received by the sender.

miri64 · 2017-09-06T16:04:38Z

It makes it a little bit more complicated, but will do.

zhuoshuguo · 2017-09-07T08:59:12Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

+    const uint8_t seq = ieee802154_get_seq(mhr);
+
+    return  (dev->last_pkt.seq == seq) &&
+            (((dev->last_pkt.was_bcast) && (netif->flags & GNRC_NETIF_HDR_FLAGS_BROADCAST)) ||


Maybe for broadcast packet, you should also check the source address, since in case two different senders (A, then B ) successively broadcast two different packets to C (with the same sequence, this time, the possibility is 1/256), then B's bcast packet will be dropped. ;-)

I do check the source address.

Ah, I see, and sorry for the noise.

zhuoshuguo · 2017-09-07T09:05:10Z

A simple solution maybe: you don't need to count whether the received packet is bcast or not, just record the source address and sequence information (don't care about destination address), and use them to judge duplication.

miri64 · 2017-09-07T09:45:31Z

A simple solution maybe: you don't need to count whether the received packet is bcast or not, just record the source address and sequence information (don't care about destination address), and use them to judge duplication.

True

miri64 · 2017-10-12T08:38:00Z

Rebased and no longer dependent on any PR, but please DO NOT MERGE yet!

jnohlgard

Some comments on the implementation. I think this is a crucial component for the network stack, but the implementation is more complex than it has to be. For IEEE 802.15.4, it should be enough to just check the sender link layer address and the sequence number to identify duplicates.

jnohlgard · 2017-10-12T11:04:48Z

drivers/include/net/netdev/ieee802154.h

    /** @} */
+#ifdef MODULE_GNRC_NETIF2_DEDUP_BCAST
+    struct {
+        uint8_t dst[IEEE802154_LONG_ADDRESS_LEN];   /**< destination address */


I don't see any reason why you should need to store the destination information. Each sender should not reuse sequence numbers except for retransmissions (and after 256 packets when the counter rolls over)

jnohlgard · 2017-10-12T11:05:15Z

drivers/include/net/netdev/ieee802154.h

+        uint8_t src[IEEE802154_LONG_ADDRESS_LEN];   /**< source address */
+        uint8_t dst_len;                            /**< destination address length */
+        uint8_t src_len;                            /**< source address length */
+        uint8_t was_bcast;                          /**< indicates if dst was broadcast */


Why is this relevant for deduplication?

jnohlgard · 2017-10-12T11:06:29Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

+            if (_already_received(dev, hdr, ieee802154_hdr->data)) {
+                gnrc_pktbuf_release(pkt);
+                gnrc_pktbuf_release(netif_hdr);
+                DEBUG("_recv_ieee802154: packet dropped by broadcast deduplication\n");


Scratch " broadcast " if this applies to all packets now.

jnohlgard · 2017-10-12T11:07:14Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

+    return  (dev->last_pkt.seq == seq) &&
+            (((dev->last_pkt.was_bcast) && (netif->flags & GNRC_NETIF_HDR_FLAGS_BROADCAST)) ||
+             ((dev->last_pkt.dst_len == netif->dst_l2addr_len) &&
+              (memcmp(dev->last_pkt.dst, gnrc_netif_hdr_get_dst_addr(netif),


I see no reason why this check is necessary

miri64 · 2017-10-12T12:30:29Z

Addressed comments.

jnohlgard

Some left overs in the implementation referring to was_bcast and dst

jnohlgard · 2017-10-12T12:45:29Z

drivers/include/net/netdev/ieee802154.h

    uint8_t chan;                           /**< channel */
    uint16_t flags;                         /**< flags as defined above */
    /** @} */
+#ifdef MODULE_GNRC_NETIF2_DEDUP_BCAST


Module name could be just gnrc_netif2_dedup

Rememeber what I said about coffee ^^

jnohlgard · 2017-10-12T12:46:48Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

+                DEBUG("_recv_ieee802154: packet dropped by deduplication\n");
+                return NULL;
+            }
+            dev->last_pkt.was_bcast = ((netif->flags & GNRC_NETIF_HDR_FLAGS_BROADCAST) != 0);


This probably doesn't compile any more

miri64 · 2017-10-12T12:58:53Z

Addressed comments. Just a friendly reminder to please not merged, before the whole netif2 situation on the maintainers list was discussed.

jnohlgard · 2017-10-12T13:05:08Z

Yep, I will let you do the merging when you feel it is time.

jnohlgard

Looks much cleaner now, just need to update the preprocessor condition. The commit message and PR title need updating as well.

jnohlgard · 2017-10-12T13:06:04Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

    return snip;
 }

+#if MODULE_GNRC_NETIF2_DEDUP_BCAST


Need to update this module name

Arghs... Need more coffee, I think ^^

jnohlgard · 2017-10-12T13:06:16Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

                return NULL;
            }
 #endif
+#ifdef MODULE_GNRC_NETIF2_DEDUP_BCAST


Update this

miri64 · 2017-10-12T13:09:44Z

Done.

zhuoshuguo · 2017-10-16T17:54:45Z

Checked the codes, also look fine to me. ;-)

jnohlgard

Looks good except for a small typo.
Please squash

jnohlgard · 2017-11-04T22:12:39Z

sys/net/gnrc/netif2/gnrc_netif2_ieee802154.c

+            }
+            memcpy(dev->last_pkt.src, gnrc_netif_hdr_get_src_addr(hdr));
+            dev->last_pkt.src_len = hdr->src_l2addr_len;
+            dev->seq = seq;


It seems like this should be dev->last_pkt.seq = seq

miri64 · 2017-11-06T06:00:56Z

Fixed and squashed

miri64 · 2017-11-06T06:01:57Z

Please wait with merging until #7925 is merged (alternatively we can merge this PR into that, I need to rebase then).

PeterKietzmann · 2018-12-18T17:25:47Z

Just by following the comments it seems as if we're good to go? Did someone run any test?

jnohlgard · 2018-12-18T17:56:17Z

I did a very long time ago. I think i5 would be wise to test again

miri64 · 2018-12-18T20:55:27Z

Made compilable again

PeterKietzmann · 2018-12-18T21:13:24Z

I did some basic testing:

examples/gnrc_networking on two samr21-xpro boards
compiled with USEMODULE=gnrc_netif_dedup
enabled debug messages in gnrc_netif_ieee802154.c
disable autoack feature on board A
ping6 from board A to board B
look at the following outputs

current master

# ping6 1 fe80::7b62:1b6d:89cc:89ca
# _recv_ieee802154: received packet from 79:62:1B:6D:89:CC:89:CA of length 15
# 00000000  7A  33  3A  81  00  7C  9B  00  5C  00  01  5C  5C  5C  5C
# _recv_ieee802154: reallocating.
# _recv_ieee802154: received packet from 79:62:1B:6D:89:CC:89:CA of length 15
# 00000000  7A  33  3A  81  00  7C  9B  00  5C  00  01  5C  5C  5C  5C
# _recv_ieee802154: reallocating.
# _recv_ieee802154: received packet from 79:62:1B:6D:89:CC:89:CA of length 15
# 00000000  7A  33  3A  81  00  7C  9B  00  5C  00  01  5C  5C  5C  5C
# _recv_ieee802154: reallocating.
# 12 bytes from fe80::7b62:1b6d:89cc:89ca: id=92 seq=1 hop limit=64 time = 89.876 ms
# dropping additional response packet (probably caused by duplicates)
# dropping additional response packet (probably caused by duplicates)
# --- fe80::7b62:1b6d:89cc:89ca ping statistics ---
# 1 packets transmitted, 1 received, 0% packet loss, time 0.06109588 s
# rtt min/avg/max = 89.876/89.876/89.876 ms

with this PR

#  ping6 1  fe80::7b62:1b6d:89cc:89ca
# _recv_ieee802154: received packet from 79:62:1B:6D:89:CC:89:CA of length 15
# 00000000  7B  33  3A  81  00  8E  B6  00  53  00  01  53  53  53  53
# _recv_ieee802154: reallocating.
# _recv_ieee802154: packet dropped by deduplication
# 12 bytes from fe80::7b62:1b6d:89cc:89ca: id=83 seq=1 hop limit=255 time = 45.776 ms
# --- fe80::7b62:1b6d:89cc:89ca ping statistics ---
# 1 packets transmitted, 1 received, 0% packet loss, time 0.0653424 s
# rtt min/avg/max = 45.776/45.776/45.776 ms

-> seems to work! For some reason I don't see all retransmissions I would expect to be sent by board B. Probably some timing issues...

miri64 · 2018-12-18T21:21:48Z

For some reason I don't see all retransmissions I would expect to be sent by board B. Probably some timing issues...

I'd say this is unrelated to this PR.

PeterKietzmann · 2019-01-23T10:28:49Z

I think this is a useful feature although it might make sense to have sort of documentation for the pseudomodule. @aabadie whats your opinion to get this in for the release?

PeterKietzmann · 2019-01-30T08:07:48Z

@miri64, @aabadie, @gebart what's your opinion here? With some form of documentation I'd ACK this PR. Needs rebase btw...

miri64 · 2019-02-01T14:29:39Z

I'd be happy to have it in ;-). Will rebase.

miri64 · 2019-02-01T14:32:30Z

Done

PeterKietzmann · 2019-02-04T08:11:29Z

I still miss some kind of documentation. How would a user be aware of that module? Don't know what's the best location, maybe doxygen in net/gnrc/netif/ieee802154.h?

miri64 · 2019-02-04T08:22:50Z

I still miss some kind of documentation. How would a user be aware of that module? Don't know what's the best location, maybe doxygen in net/gnrc/netif/ieee802154.h?

Now that you mention it, it doesn't really make sense to store the data in netdev_ieee802154, since all the implementation logic is happening in gnrc_netif (the inclusion of the data even is dependent on **gnrc_**netif). Will fix that as well with the added doc.

PeterKietzmann · 2019-02-04T08:31:54Z

Good!

miri64

Ported to a pure gnrc_netif approach and also added some documentation.

miri64 · 2019-02-04T11:38:07Z

sys/include/net/gnrc/netif/dedup.h

+ */
+typedef struct {
+    uint8_t src[GNRC_NETIF_L2ADDR_MAXLEN];  /**< link-layer source address */
+    uint16_t seq;                           /**< link-layer sequence number */


FYI I chose uint16_t instead of uint8_t since this implementation supposed to be independent of IEEE 802.15.4. I'm not aware of any packet switching Link-Layer with packets >UINT16_MAX so I figured uint16_t might be enough.

PeterKietzmann

@miri64 I re-tested with the procedure described above, still works as expected. Please squash. @gebart would you remove your change request?

PeterKietzmann · 2019-02-04T15:15:13Z

sys/net/gnrc/netif/ieee802154/gnrc_netif_ieee802154.c

+#if MODULE_GNRC_NETIF_DEDUP
+static inline bool _already_received(gnrc_netif_t *netif,
+                                     gnrc_netif_hdr_t *netif_hdr,
+                                     uint8_t *mhr)


Seeing this makes me wonder a bit about the netif data structures, although not related to this PR...

What exactly are you wondering about?

About that fact that we need three "data structures" in order to get header information such as sequence number or link layer addresses.

About that fact that we need three "data structures" in order to get header information such as sequence number or link layer addresses.

For that we "only" need two. The third (netif) is the current state against we check that information. We wouldn't actually need two either, but the addresses in the IEEE 802.15.4 are rather complicated to parse out due to its and their variable lengths, so why not actually use the results from a previous parsing step i.e. netif_hdr (we don't need the sequence number in that for most use-cases)

miri64 · 2019-02-04T15:20:11Z

@miri64 I re-tested with the procedure described above, still works as expected. Please squash.

squashed

PeterKietzmann

ACK from my side. But I'm giving @gebart some time to double-check or at least remove his change request.

Not responsive

PeterKietzmann · 2019-02-11T08:41:45Z

I dismissed @gebart's review as he's been unavailable for quite some time. @gebart please give me a sign if you disagree with this PR. I'll handle the follow-ups then. Otherwise: ACK and go

miri64 added GNRC Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 6, 2017

miri64 assigned jnohlgard and zhuoshuguo Sep 6, 2017

miri64 requested a review from jnohlgard September 6, 2017 15:20

miri64 mentioned this pull request Sep 6, 2017

LWMAC: avoid receiving duplicate broadcast packet. #7442

Closed

zhuoshuguo reviewed Sep 7, 2017

View reviewed changes

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 12, 2017

miri64 force-pushed the gnrc_netif2/feat/ieee802154-dup-bcast branch from 9abf3e1 to e8b52cf Compare October 12, 2017 08:37

jnohlgard requested changes Oct 12, 2017

View reviewed changes

miri64 mentioned this pull request Oct 13, 2017

RFC: A layered approach to netdev #7736

Open

jnohlgard requested changes Nov 4, 2017

View reviewed changes

miri64 force-pushed the gnrc_netif2/feat/ieee802154-dup-bcast branch from 914c4e6 to 267d08d Compare November 6, 2017 06:00

miri64 force-pushed the gnrc_netif2/feat/ieee802154-dup-bcast branch from 267d08d to db69497 Compare November 27, 2017 20:23

miri64 force-pushed the gnrc_netif2/feat/ieee802154-dup-bcast branch from 443c3a9 to 9e69c4a Compare December 18, 2018 15:09

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 18, 2018

miri64 changed the title ~~gnrc_netif2_ieee802154: drop duplicate broadcast packets (optionally)~~ gnrc_netif_ieee802154: drop duplicate broadcast packets (optionally) Dec 18, 2018

PeterKietzmann added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 7, 2019

miri64 force-pushed the gnrc_netif2/feat/ieee802154-dup-bcast branch from be82f4f to 4e870b4 Compare February 1, 2019 14:32

PeterKietzmann added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Feb 4, 2019

miri64 commented Feb 4, 2019

View reviewed changes

PeterKietzmann reviewed Feb 4, 2019

View reviewed changes

gnrc_netif_ieee802154: drop duplicate broadcast packets (optionally)

985d980

miri64 force-pushed the gnrc_netif2/feat/ieee802154-dup-bcast branch from 3103bdb to 985d980 Compare February 4, 2019 15:19

PeterKietzmann approved these changes Feb 4, 2019

View reviewed changes

PeterKietzmann merged commit 63fd8bc into RIOT-OS:master Feb 11, 2019

miri64 deleted the gnrc_netif2/feat/ieee802154-dup-bcast branch February 11, 2019 08:45

miri64 added this to the Release 2019.04 milestone Feb 18, 2019

Conversation

miri64 commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Sep 6, 2017

Uh oh!

miri64 commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuoshuguo commented Sep 7, 2017

Uh oh!

miri64 commented Sep 7, 2017

Uh oh!

miri64 commented Oct 12, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 12, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 12, 2017

Uh oh!

jnohlgard commented Oct 12, 2017

Uh oh!

jnohlgard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 12, 2017

Uh oh!

zhuoshuguo commented Oct 16, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 6, 2017

Uh oh!

miri64 commented Nov 6, 2017

Uh oh!

PeterKietzmann commented Dec 18, 2018

Uh oh!

jnohlgard commented Dec 18, 2018

Uh oh!

miri64 commented Dec 18, 2018

Uh oh!

PeterKietzmann commented Dec 18, 2018

Uh oh!

miri64 commented Dec 18, 2018

Uh oh!

PeterKietzmann commented Jan 23, 2019

miri64 commented Sep 6, 2017 •

edited

Loading

jnohlgard left a comment •

edited

Loading