Re: [PATCH v11 2/9] fwd: Add cache table for ARP/NDP contents

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]

On Tue, Sep 30, 2025 at 01:58:42AM +0200, Stefano Brivio wrote:
> Almost entirely nitpicks here:
> 
> On Sat, 27 Sep 2025 15:25:15 -0400
> Jon Maloy <jmaloy@redhat.com> wrote:
> 
> > We add a cache table to keep track of the contents of the kernel ARP
> > and NDP tables. The table is fed from the just introduced netlink based
> > neigbour subscription function. The new table eliminates the need for
> > explicit netlink calls to find a host's MAC address.
> > 
> > Signed-off-by: Jon Maloy <jmaloy@redhat.com>

[snip]
> > +				      const union inany_addr *key)
> > +{
> > +	struct siphash_state st = SIPHASH_INIT(c->hash_secret);
> > +	uint32_t i;
> > +
> > +	inany_siphash_feed(&st, key);
> > +	i = siphash_final(&st, sizeof(*key), 0);
> > +
> > +	return ((size_t)i) & (NEIGH_TABLE_SIZE - 1);
> > +}
> > +
> > +/**
> > + * fwd_neigh_table_find() - Find a MAC table entry
> 
> Strictly speaking, it's a MAC address table -- MAC refers to the access
> control itself.

I mean, it's both.  The entries are our neighbours and the contents of
each entry is the MAC address.

[snip]
> > +void fwd_neigh_table_free(const struct ctx *c, const union inany_addr *addr)
> > +{
> > +	ssize_t slot = neigh_table_slot(c, addr);
> > +	struct neigh_table *t = &neigh_table;
> > +	struct neigh_table_entry *e, **prev;
> > +
> > +	prev = &t->slots[slot];
> > +	e = t->slots[slot];
> > +	while (e && !inany_equals(&e->addr, addr)) {
> > +		prev = &e->next;
> > +		e = e->next;
> > +	}
> > +	if (!e)
> > +		return;
> > +
> > +	*prev = e->next;
> > +	e->next = t->free;
> > +	t->free = e;
> > +	memset(&e->addr, 0, sizeof(*addr));
> > +	memset(e->mac, 0, ETH_ALEN);
> 
> Do we care about zeroing them? If we do because we might find those
> entries, note that both all-zero MAC address and IP addresses are
> valid.

In the case of IP addresses, while all-zero is valid in certain
contexts, I'd argue it's never a valid address for a neighbour ("this
host on this network" is frustratingly vague, but pretty clearly not a
neighbour).  255.255.255.255 would also make a reasonable placeholder
if we must have one.

For MAC addresses, ff:ff:ff:ff:ff:ff would probably make a better
placeholder, if we must have one.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]