Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnrc/ipv6/nib: don't queue packets on 6lo neighbors and drop/flush if… #20834

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions sys/include/net/gnrc/ipv6/nib/conf.h
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,18 @@ extern "C" {
#define CONFIG_GNRC_IPV6_NIB_NUMOF (4)
#endif

/**
* @brief Per-neighbor packet queue capacity
*
* If @ref CONFIG_GNRC_IPV6_NIB_QUEUE_PKT enabled, this is the maximum number
* of packets, per neighbor, awaiting packet resolution.
*
* @attention This MUST be leq UINT8_MAX
*/
#ifndef CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP
#define CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP (16)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to have the default be so high?

Copy link
Contributor Author

@derMihai derMihai Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This disables some code when the max neighbor count < CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP, which is so per default. With a large neighbor count, we mainly want to prevent a neighbor depleting the queue - for performance reasons - and in that case it's not that big anymore.

This was my only rationale when picking 16.

#endif

/**
* @brief Number of off-link entries in NIB
*
Expand Down
18 changes: 12 additions & 6 deletions sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.c
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,7 @@ void _handle_snd_ns(_nib_onl_entry_t *nbr)
case GNRC_IPV6_NIB_NC_INFO_NUD_STATE_PROBE:
if (nbr->ns_sent >= NDP_MAX_UC_SOL_NUMOF) {
gnrc_netif_t *netif = gnrc_netif_get_by_pid(_nib_onl_get_if(nbr));

_set_nud_state(netif, nbr,
GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE);
_set_unreachable(netif, nbr);
}
/* intentionally falls through */
case GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE:
Expand Down Expand Up @@ -387,11 +385,10 @@ void _handle_adv_l2(gnrc_netif_t *netif, _nib_onl_entry_t *nce,
nce->info &= ~GNRC_IPV6_NIB_NC_INFO_IS_ROUTER;
}
}
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT) && MODULE_GNRC_IPV6
/* send queued packets */
gnrc_pktqueue_t *ptr;
DEBUG("nib: Sending queued packets\n");
while ((ptr = gnrc_pktqueue_remove_head(&nce->pktqueue)) != NULL) {
while ((ptr = _nbr_pop_pkt(nce)) != NULL) {
if (!gnrc_netapi_dispatch_send(GNRC_NETTYPE_IPV6,
GNRC_NETREG_DEMUX_CTX_ALL,
ptr->pkt)) {
Expand All @@ -400,7 +397,7 @@ void _handle_adv_l2(gnrc_netif_t *netif, _nib_onl_entry_t *nce,
}
ptr->pkt = NULL;
}
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */

if ((icmpv6->type == ICMPV6_NBR_ADV) &&
!_sflag_set((ndp_nbr_adv_t *)icmpv6) &&
(_get_nud_state(nce) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_REACHABLE) &&
Expand Down Expand Up @@ -439,6 +436,15 @@ void _set_reachable(gnrc_netif_t *netif, _nib_onl_entry_t *nce)
netif->ipv6.reach_time);
}

void _set_unreachable(gnrc_netif_t *netif, _nib_onl_entry_t *nce)
{
DEBUG("nib: set %s to UNREACHABLE\n",
ipv6_addr_to_str(addr_str, &nce->ipv6, sizeof(addr_str)));

_nbr_flush_pktqueue(nce);
derMihai marked this conversation as resolved.
Show resolved Hide resolved
_set_nud_state(netif, nce, GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE);
}

void _set_nud_state(gnrc_netif_t *netif, _nib_onl_entry_t *nce,
uint16_t state)
{
Expand Down
12 changes: 10 additions & 2 deletions sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,22 @@ void _handle_adv_l2(gnrc_netif_t *netif, _nib_onl_entry_t *nce,
void _recalc_reach_time(gnrc_netif_ipv6_t *netif);

/**
* @brief Sets a neighbor cache entry reachable and starts the required
* @brief Sets a neighbor cache entry REACHABLE and starts the required
* event timers
*
* @param[in] netif Interface to the NCE
* @param[in] nce The neighbor cache entry to set reachable
* @param[in] nce The neighbor cache entry to set REACHABLE
*/
void _set_reachable(gnrc_netif_t *netif, _nib_onl_entry_t *nce);

/**
* @brief Sets a neighbor cache entry UNREACHABLE and flushes its packet queue
*
* @param[in] netif Interface to the NCE
* @param[in] nce The neighbor cache entry to set UNREACHABLE
*/
void _set_unreachable(gnrc_netif_t *netif, _nib_onl_entry_t *nce);

/**
* @brief Initializes interface for address registration state machine
*
Expand Down
13 changes: 1 addition & 12 deletions sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -276,18 +276,7 @@ void _nib_nc_remove(_nib_onl_entry_t *node)
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LR)
evtimer_del((evtimer_t *)&_nib_evtimer, &node->addr_reg_timeout.event);
#endif /* CONFIG_GNRC_IPV6_NIB_6LR */
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
gnrc_pktqueue_t *tmp;
for (gnrc_pktqueue_t *ptr = node->pktqueue;
(ptr != NULL) && (tmp = (ptr->next), 1);
ptr = tmp) {
gnrc_pktqueue_t *entry = gnrc_pktqueue_remove(&node->pktqueue, ptr);
gnrc_icmpv6_error_dst_unr_send(ICMPV6_ERROR_DST_UNR_ADDR,
entry->pkt);
gnrc_pktbuf_release_error(entry->pkt, EHOSTUNREACH);
entry->pkt = NULL;
}
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
_nbr_flush_pktqueue(node);
/* remove from cache-out procedure */
clist_remove(&_next_removable, (clist_node_t *)node);
_nib_onl_clear(node);
Expand Down
38 changes: 38 additions & 0 deletions sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ typedef struct _nib_onl_entry {
*/
uint8_t l2addr_len;
#endif
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT) || defined(DOXYGEN)
uint8_t pktqueue_len; /**< Number of queued packets (in pktqueue) */
#endif
} _nib_onl_entry_t;

/**
Expand Down Expand Up @@ -872,6 +875,41 @@ void _nib_ft_get(const _nib_offl_entry_t *dst, gnrc_ipv6_nib_ft_t *fte);
int _nib_get_route(const ipv6_addr_t *dst, gnrc_pktsnip_t *ctx,
gnrc_ipv6_nib_ft_t *entry);

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT) || DOXYGEN
/**
* @brief Flush the packet queue of a on-link neighbor.
*
* @param node neighbor entry to be flushed
*/
void _nbr_flush_pktqueue(_nib_onl_entry_t *node);

/**
* @brief Remove oldest packet from a on-link neighbor's packet queue.
*
* @param node neighbor entry
*
* @retval pointer to the packet entry or NULL if the queue is empty
*/
gnrc_pktqueue_t *_nbr_pop_pkt(_nib_onl_entry_t *node);

/**
* @brief Push packet to a on-link neighbor's packet queue.
*
* If there are already @ref CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP packets queued,
* the oldest will be dropped silently.
*
* @pre Neighbor is INCOMPLETE.
*
* @param node neighbor entry
* @param pkt packet to be pushed
*/
void _nbr_push_pkt(_nib_onl_entry_t *node, gnrc_pktqueue_t *pkt);
#else
#define _nbr_flush_pktqueue(node) ((void)node)
#define _nbr_pop_pkt(node) ((void)node, NULL)
#define _nbr_push_pkt(node, pkt) ((void)node, (void)pkt)
#endif
benpicco marked this conversation as resolved.
Show resolved Hide resolved

#ifdef __cplusplus
}
#endif
Expand Down
157 changes: 138 additions & 19 deletions sys/net/gnrc/network_layer/ipv6/nib/nib.c
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,20 @@
static char addr_str[IPV6_ADDR_MAX_STR_LEN];

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
static gnrc_pktqueue_t _queue_pool[CONFIG_GNRC_IPV6_NIB_NUMOF];
/* +1 ensures that whenever the pool is empty, there is at least one neighbor
* with 2 or more packets, thus we can always pop a packet from that neighbor
* without leaving it's queue empty, as required by
*
* https://www.rfc-editor.org/rfc/rfc4861#section-7.2.2
*
* While waiting for address resolution to complete, the sender MUST,
* for each neighbor, retain a small queue of packets waiting for
* address resolution to complete. The queue MUST hold at least one
* packet, and MAY contain more. However, the number of queued packets
* per neighbor SHOULD be limited to some small value. When a queue
* overflows, the new arrival SHOULD replace the oldest entry. Once
* address resolution completes, the node transmits any queued packets. */
static gnrc_pktqueue_t _queue_pool[CONFIG_GNRC_IPV6_NIB_NUMOF + 1];
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_DNS)
Expand Down Expand Up @@ -549,7 +562,7 @@
"is not a forwarding interface). Discarding silently\n",
netif->pid);
DEBUG(" - IP Hop Limit: %u (should be %u)\n", ipv6->hl,
NDP_HOP_LIMIT);

Check warning on line 565 in sys/net/gnrc/network_layer/ipv6/nib/nib.c

View workflow job for this annotation

GitHub Actions / static-tests

full block {} expected in the control structure
DEBUG(" - ICMP code: %u (should be 0)\n", rtr_sol->code);
DEBUG(" - ICMP length: %" PRIuSIZE " (should > %" PRIuSIZE ")\n",
icmpv6_len, sizeof(ndp_rtr_sol_t));
Expand Down Expand Up @@ -1267,19 +1280,77 @@
}
}

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
static _nib_onl_entry_t *_iter_nc_nbr(_nib_onl_entry_t const *last)
{
while ((last = _nib_onl_iter(last))) {
if (last->mode & _NC) {
break;
}
}

return (_nib_onl_entry_t *)last;
}
#endif

/* This function never fails as doing so would force us to drop newer packets
* instead of older, thus leaving stale packets in the neighbor queues.
*
* https://www.rfc-editor.org/rfc/rfc4861#section-7.2.2
*
* While waiting for address resolution to complete, the sender MUST,
* for each neighbor, retain a small queue of packets waiting for
* address resolution to complete. The queue MUST hold at least one
* packet, and MAY contain more. However, the number of queued packets
* per neighbor SHOULD be limited to some small value. When a queue
* overflows, the new arrival SHOULD replace the oldest entry. Once
* address resolution completes, the node transmits any queued packets. */
static gnrc_pktqueue_t *_alloc_queue_entry(gnrc_pktsnip_t *pkt)
{
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
for (int i = 0; i < CONFIG_GNRC_IPV6_NIB_NUMOF; i++) {
for (size_t i = 0; i < ARRAY_SIZE(_queue_pool); i++) {
if (_queue_pool[i].pkt == NULL) {
_queue_pool[i].pkt = pkt;
return &_queue_pool[i];
}
}

/* We run out of free queue entries. Pop from the nbr with the longest queue */
_nib_onl_entry_t *nbr = _iter_nc_nbr(NULL);
_nib_onl_entry_t *hog = nbr;
/* There MUST be at least a neighbor in the NC */
assert(hog);
while ((nbr = _iter_nc_nbr(nbr))) {
if (ARRAY_SIZE(_queue_pool) >= CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP &&
/* The per-neighbor queue is capped at CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP.
* There cannot be a larger hog than that. */
hog->pktqueue_len == CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP) {
break;
}
assert(hog->pktqueue_len < CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP);

if (nbr->pktqueue_len > hog->pktqueue_len) {
hog = nbr;
}
}

DEBUG("nib: no free pktqueue entries, popping from %s hogging %u\n",
ipv6_addr_to_str(addr_str, &hog->ipv6, sizeof(addr_str)),
hog->pktqueue_len);

/* We have one more pktqueue entries than neighbors in the NC, therefore
* there must be a neighbor with two or more packets in its queue */
assert(hog->pktqueue_len >= 2);

gnrc_pktqueue_t *qentry = _nbr_pop_pkt(hog);
gnrc_pktbuf_release(qentry->pkt);

qentry->pkt = pkt;
return qentry;
#else
(void)pkt;
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
return NULL;
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
}

static bool _resolve_addr_from_nc(_nib_onl_entry_t *entry, gnrc_netif_t *netif,
Expand Down Expand Up @@ -1317,13 +1388,6 @@

gnrc_pktqueue_t *queue_entry = _alloc_queue_entry(pkt);

if (queue_entry == NULL) {
DEBUG("nib: can't allocate entry for packet queue "
"dropping packet\n");
gnrc_pktbuf_release(pkt);
return false;
}

if (netif != NULL) {
gnrc_pktsnip_t *netif_hdr = gnrc_netif_hdr_build(NULL, 0, NULL, 0);

Expand All @@ -1337,11 +1401,7 @@
queue_entry->pkt = gnrc_pkt_prepend(queue_entry->pkt, netif_hdr);
}

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
gnrc_pktqueue_add(&entry->pktqueue, queue_entry);
#else
(void)entry;
#endif
_nbr_push_pkt(entry, queue_entry);
return true;
}

Expand Down Expand Up @@ -1373,6 +1433,16 @@
return true;
}

/* don't do multicast address resolution on 6lo */
if (gnrc_netif_is_6ln(netif)) {
/* https://www.rfc-editor.org/rfc/rfc6775.html#section-5.6
* A LoWPAN node is not required to maintain a minimum of one buffer
* per neighbor as specified in [RFC4861], since packets are never
* queued while waiting for address resolution. */
gnrc_pktbuf_release(pkt);
return false;
}

bool reset = false;
DEBUG("nib: resolve address %s by probing neighbors\n",
ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
Expand Down Expand Up @@ -1404,10 +1474,7 @@
return false;
}

/* don't do multicast address resolution on 6lo */
if (!gnrc_netif_is_6ln(netif)) {
_probe_nbr(entry, reset);
}
_probe_nbr(entry, reset);

return false;
}
Expand Down Expand Up @@ -1733,4 +1800,56 @@

return route_ltime;
}

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
gnrc_pktqueue_t *_nbr_pop_pkt(_nib_onl_entry_t *node)
{
if (node->pktqueue_len == 0) {
assert(node->pktqueue == NULL);
return NULL;
}
assert(node->pktqueue != NULL);

node->pktqueue_len--;

return gnrc_pktqueue_remove_head(&node->pktqueue);
}

void _nbr_push_pkt(_nib_onl_entry_t *node, gnrc_pktqueue_t *pkt)
{
static_assert(CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP <= UINT8_MAX,
"nib: nbr queue cap overflows counter");
assert(_get_nud_state(node) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
/* We're capping the per-neighbor queue length out of following reasons:
* - https://www.rfc-editor.org/rfc/rfc4861#section-7.2.2 recommends a
* small queue size
* - for large CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP, a single neighbor could
* otherwise consume the whole entry cache. By capping we rule out this
* case, thus: 1) a hog will just drop from it's own queue and 2) there's
* less likely to deplete the entry cache.
*
* For small CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP we don't care how the entries
* are distributed: if we run out of entries, finding the hog to drop from
* there is fast anyway. */
if (ARRAY_SIZE(_queue_pool) > CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP &&
node->pktqueue_len == CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP) {
Comment on lines +1834 to +1835
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (ARRAY_SIZE(_queue_pool) > CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP &&
node->pktqueue_len == CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP) {
if (node->pktqueue_len == (ARRAY_SIZE(_queue_pool) - 1) ||
node->pktqueue_len == CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP) {

I thought you wanted to prevent one node from hogging the entire queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're talking past each other here. Let's recap:

  • we have CONFIG_GNRC_IPV6_NIB_NUMOF + 1 queue entries in the cache s.t. when they're used up there's always one neighbor with at least two
  • with CONFIG_GNRC_IPV6_NIB_NUMOF small ( ~ 16) we really don't care how they are distributed. Once we're out of entries, find the hog and take from there. This is fast, because we have at most ~16 neighbors and at most ~16 entries. The check ARRAY_SIZE(_queue_pool) > CONFIG_GNRC_IPV6_NIB_NBR_QUEUE_CAP compiles the cap-check away.
  • with CONFIG_GNRC_IPV6_NIB_NUMOF big, we want a per-neighbor cap, which defaults at 16, out of the reasons stated previously:
    • A single hog cannot deplete the entry cache by itself -> less cases where we have to iterate all neighbors to find a hog
    • RFC recommends keeping the per-neighbor queue small (whatever is considered small)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah makes perfect sense!
But since I had trouble getting it the first time, better also add a comment to the code to prevent misunderstandings with the next one reading this.

gnrc_pktqueue_t *oldest = _nbr_pop_pkt(node);
gnrc_pktbuf_release(oldest->pkt);
Copy link
Contributor

@benpicco benpicco Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not gnrc_pktbuf_release_error(oldest->pkt, ENOMEM)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not. You can always send a packet by dropping older ones from a queue, and I think this is arguably normal operation. Also, in this case here we're not necessary out of queue entries, but reaching the per-neighbor limit.

oldest->pkt = NULL;
}

gnrc_pktqueue_add(&node->pktqueue, pkt);
node->pktqueue_len++;
}

void _nbr_flush_pktqueue(_nib_onl_entry_t *node)
{
gnrc_pktqueue_t *entry;
while ((entry = _nbr_pop_pkt(node))) {
gnrc_icmpv6_error_dst_unr_send(ICMPV6_ERROR_DST_UNR_ADDR, entry->pkt);
gnrc_pktbuf_release_error(entry->pkt, EHOSTUNREACH);
entry->pkt = NULL;
}
}
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
/** @} */
Loading