[riot-notifications] [RIOT-OS/RIOT] gnrc_sixlowpan_frag: initial import of minimal forwarding (#11068)

benpicco notifications at github.com
Sun Nov 29 01:19:43 CET 2020


@benpicco commented on this pull request.

I'm not familiar with the protocol, so only some nits and questions to better understand the code, where some code comments would also be beneficial for future readers.

Some general questions:
 - how many reassembly buffers can there?  
 - Will we wait a bit before sending the next fragment, so not to immediately cause a collision with the retransmission from the next hop?

I didn't check yet, but I guess there is no effect on code size / behavior if the module is *not* used.

> +                    gnrc_sixlowpan_frag_vrb_t *vrbe;
+                    gnrc_pktsnip_t tmp = {
+                        .data = data,
+                        .size = frag_size,
+                        .users = 1,
+                    };

needs one more level of indentation 

> @@ -303,6 +350,47 @@ static int _rbuf_add(gnrc_netif_hdr_t *netif_hdr, gnrc_pktsnip_t *pkt,
             if (data[0] == SIXLOWPAN_UNCOMP) {
                 DEBUG("6lo rbuf: detected uncompressed datagram\n");
                 data++;
+                if (IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_MINFWD) &&
+                    /* only try minimal forwarding when fragment is the only
+                     * fragment in reassembly buffer yet */
+                    sixlowpan_frag_1_is(pkt->data) &&
+                    (entry.super->current_size == frag_size)) {

This does not trigger if the packet just happens to fit exactly into one 6lo frame, right?

> +                        _adapt_hdr(&tmp, page);
+                        int res = _forward_frag(pkt, sizeof(sixlowpan_frag_t),
+                                                vrbe, page);
+
+                        /* prevent intervals from being deleted (they are in the
+                         * VRB now) */
+                        entry.rbuf->super.ints = NULL;
+                        gnrc_pktbuf_release(entry.rbuf->pkt);
+                        gnrc_sixlowpan_frag_rb_remove(entry.rbuf);
+                        return (res == 0) ? RBUF_ADD_SUCCESS : RBUF_ADD_ERROR;
+                    }
+                }
+            }
+        }
+        if (IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_MINFWD)) {
+            DEBUG("6lo rbuf: just do normal reassembly\n");

As opposed to abnormal reassembly?
Sorry, I have a hard time following the level of nesting we are on here

> @@ -504,7 +592,11 @@ static int _rbuf_get(const void *src, size_t src_len,
         default:
             reass_type = GNRC_NETTYPE_UNDEF;
     }
+#ifdef MODULE_GNRC_SIXLOWPAN_FRAG_VRB

for consistency you might also want to use

```C
#if IS_USED(MODULE_GNRC_NETTYPE_IPV6)
```

here

(Although I think `#ifdef MODULE_GNRC_SIXLOWPAN_FRAG_VRB` is much clearer and `IS_USED()` should only be used in C conditionals, not preprocessor conditionals) 

> @@ -642,5 +734,68 @@ int gnrc_sixlowpan_frag_rb_dispatch_when_complete(gnrc_sixlowpan_frag_rb_t *rbuf
     return res;
 }
 
+static bool _check_hdr(gnrc_pktsnip_t *hdr, unsigned page)

What is `page`? 

> +}
+
+static int _forward_frag(gnrc_pktsnip_t *pkt, size_t frag_hdr_size,
+                         gnrc_sixlowpan_frag_vrb_t *vrbe, unsigned page)
+{
+    int res = -ENOTSUP;
+
+    if (IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_MINFWD)) {
+        gnrc_pktsnip_t *frag = gnrc_pktbuf_mark(pkt, frag_hdr_size,
+                                                GNRC_NETTYPE_SIXLOWPAN);
+        if (frag == NULL) {
+            gnrc_pktbuf_release(pkt);
+            res = -ENOMEM;
+        }
+        else {
+            LL_DELETE(pkt, frag);

I think you recently replaced those with `gnrc_` list functions 

> +    const size_t fragsnip_size = sizeof(*frag) -
+                                 ((sixlowpan_frag_1_is((sixlowpan_frag_t *)frag))
+                                 ? sizeof(frag->offset) : 0U);

What is `frag->offset` and why is it only stored in the first fragment? 

> +    tmp = _netif_hdr_from_vrbe(vrbe);
+    if (tmp == NULL) {
+        gnrc_pktbuf_release(pkt);
+        return -ENOMEM;
+    }
+    if (_is_last_frag(vrbe)) {
+        DEBUG("6lo minfwd: current_size (%u) >= datagram_size (%u)\n",
+              vrbe->super.current_size, vrbe->super.datagram_size);
+        gnrc_sixlowpan_frag_vrb_rm(vrbe);
+    }
+    else {
+        gnrc_netif_hdr_t *netif_hdr = tmp->data;
+
+        netif_hdr->flags |= GNRC_NETIF_HDR_FLAGS_MORE_DATA;
+    }
+    LL_PREPEND(pkt, tmp);

`gnrc_pkt_prepend()`?

> -    if (netif->flags & GNRC_NETIF_FLAGS_6LO_HC) {
-        gnrc_sixlowpan_iphc_send(pkt, NULL, 0);
+    if (IS_USED(MODULE_GNRC_SIXLOWPAN_IPHC) &&
+        netif->flags & GNRC_NETIF_FLAGS_6LO_HC) {
+        gnrc_sixlowpan_frag_fb_t *fbuf;
+
+        if (IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_HINT) &&
+            IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_MINFWD)) {
+            /* prepare for sending with IPHC slack in first fragment */
+            fbuf = gnrc_sixlowpan_frag_fb_get();
+            if (fbuf != NULL) {
+                fbuf->pkt = pkt;
+                fbuf->datagram_size = datagram_size;
+                fbuf->tag = gnrc_sixlowpan_frag_fb_next_tag();
+                fbuf->offset = 0;
+#if IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_HINT)

This is already checked by the encapsulating if block (line 300)

> @@ -293,12 +293,36 @@ static void _send(gnrc_pktsnip_t *pkt)
         return;
     }
 
-#ifdef MODULE_GNRC_SIXLOWPAN_IPHC
-    if (netif->flags & GNRC_NETIF_FLAGS_6LO_HC) {
-        gnrc_sixlowpan_iphc_send(pkt, NULL, 0);
+    if (IS_USED(MODULE_GNRC_SIXLOWPAN_IPHC) &&
+        netif->flags & GNRC_NETIF_FLAGS_6LO_HC) {
+        gnrc_sixlowpan_frag_fb_t *fbuf;
+
+        if (IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_HINT) &&
+            IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_MINFWD)) {
+            /* prepare for sending with IPHC slack in first fragment */

Can we already check if `!ipv6_addr_is_link_local()` here?

> @@ -1609,9 +1615,21 @@ void gnrc_sixlowpan_iphc_send(gnrc_pktsnip_t *pkt, void *ctx, unsigned page)
     gnrc_pktsnip_t *tmp;
     /* datagram size before compression */
     size_t orig_datagram_size = gnrc_pkt_len(pkt->next);
+    ipv6_hdr_t *ipv6_hdr = pkt->next->data;
+    ipv6_addr_t dst;
+
+    if (IS_USED(MODULE_GNRC_SIXLOWPAN_FRAG_MINFWD)) {
+        dst = ipv6_hdr->dst;    /* copying original destination address */
+    }
 
     (void)ctx;

guess that's no longer needed

> +        else {
+            res->pkt = gnrc_pktbuf_add(NULL, NULL, 0, reass_type);
+        }

What case is this now?

> +    else {
+        res->pkt = gnrc_pktbuf_add(NULL, NULL, size, reass_type);
+    }

And how does it differ from this one?

> @@ -852,6 +853,7 @@ void gnrc_sixlowpan_iphc_recv(gnrc_pktsnip_t *sixlo, void *rbuf_ptr,
                 }
             }
             if ((ipv6 == NULL) || (res < 0)) {
+                /* TODO fall-back to reassembly in minfwd when ipv6 != NULL */

Can you elaborate? 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11068#pullrequestreview-540424068
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201128/0d6e0dc0/attachment-0001.htm>


More information about the notifications mailing list