mirror of
https://github.com/pi-hole/FTL.git
synced 2026-08-20 06:37:31 +01:00
dnsmasq: back off MAC lookups for clients that have no ARP entry
`_FTL_new_query()` asks `find_mac()` for a client's hardware address whenever `client->hwlen` is still unset. For a client that has no ARP entry the lookup returns nothing, `hwlen` stays unset, and the next query asks again - so the lookup runs on *every* query for the lifetime of that client. That is the normal case for anything behind a router and for every loopback client, including the internal handoff FTL's own encrypted-DNS listeners use. It is not cheap. `find_mac()` may trigger a netlink call to refresh the ARP table, and the SHM lock is deliberately dropped around it so the I/O does not block other threads. Measured over 20 000 queries from a loopback client, with the per-component counters behind `debug.performance`: ``` new_query -> find_mac (netlink) 20000 calls, avg 66.8 us new_query -> lock_shm() wait after find_mac 20000 calls, avg 107.8 us new_query (total under lock) 20002 calls, avg 541.6 us ``` The lookup fired on all 20 000 queries, and re-acquiring the lock afterwards cost more than the work the unlock was protecting. Together the two account for some 27% of FTL's per-query cost, spent re-learning that the answer is still no. Remember when a lookup came back empty and skip it for a minute (`MAC_LOOKUP_BACKOFF`), rather than never: an ARP entry that appears later is still picked up on the next attempt. A client whose MAC is already known is unaffected, as `hwlen` short-circuits the check before the new one. Loopback clients are skipped outright: 127.0.0.0/8 and ::1 never appear in the neighbour table, so unlike a client that may gain an ARP entry later there is nothing to come back for, and one lookup per backoff interval would still be one too many. This adds `hwaddr_next_try` to `clientsData`, so the shared-memory version goes to 18. The field is a `uint32_t` of monotonic seconds placed in the alignment padding between `hash` and `groupspos`, so it costs nothing: measured with and without it, `sizeof(clientsData)` is 688 either way and `ippos` stays at offset 64, the cache-line start that padding exists to guarantee. `clientsData` is a per-client object held in shared memory, so a `time_t` appended to it would have cost 16 bytes each and pushed `ippos` across the boundary. Signed-off-by: DL6ER <dl6er@dl6er.de>
This commit is contained in:
+16
-6
@@ -87,12 +87,13 @@ typedef struct {
|
||||
typedef struct {
|
||||
// Hot fields ordered first for cache locality; cold overTime[] array at
|
||||
// end. Contains size_t fields -> size differs by architecture (64-bit:
|
||||
// 684 bytes for OVERTIME_SLOTS=145; 32-bit: ~668 bytes).
|
||||
// On 64-bit, 4 bytes between hash (offset 48) and groupspos (offset
|
||||
// 56) are intentional alignment padding: they ensure ippos lands at
|
||||
// offset 64, the start of cache line 1. Without them, ippos would be
|
||||
// at offset 60 and straddle the cache line boundary (bytes 60–67),
|
||||
// causing a split load on every client IP comparison.
|
||||
// 688 bytes for OVERTIME_SLOTS=145; 32-bit: smaller).
|
||||
// On 64-bit, the 4 bytes between hash (offset 48) and groupspos (offset
|
||||
// 56) were alignment padding ensuring ippos lands at offset 64, the start
|
||||
// of cache line 1; without it ippos would sit at offset 60 and straddle
|
||||
// the cache line boundary (bytes 60-67), causing a split load on every
|
||||
// client IP comparison. hwaddr_next_try now occupies that padding, so the
|
||||
// struct is the same size it always was and every offset is unchanged.
|
||||
unsigned char magic;
|
||||
char hwlen;
|
||||
unsigned char hwaddr[16]; // See DHCP_CHADDR_MAX in dnsmasq/dhcp-protocol.h
|
||||
@@ -111,6 +112,15 @@ typedef struct {
|
||||
unsigned int rate_limit;
|
||||
unsigned int numQueriesARP;
|
||||
uint32_t hash;
|
||||
// When the next MAC lookup for this client may run, in monotonic seconds.
|
||||
// A client whose address has no ARP entry - anything behind a router, and
|
||||
// every loopback client - never yields one, so without this the lookup is
|
||||
// retried on *every* query for the lifetime of the client. Deliberately
|
||||
// uint32_t and placed here: it occupies the alignment padding described
|
||||
// above, so it costs nothing per client and leaves every offset unchanged.
|
||||
// Monotonic seconds, so it is a deadline rather than a wall-clock stamp;
|
||||
// 32 bits is 136 years of uptime.
|
||||
uint32_t hwaddr_next_try;
|
||||
size_t groupspos; // SHM intarray: client's assigned group IDs
|
||||
size_t ippos;
|
||||
size_t namepos;
|
||||
|
||||
+34
-1
@@ -176,6 +176,23 @@ static uint64_t ftl_cache_misses = 0;
|
||||
#define PERF_STAT_CDB_REGEX 10 // in_regex(REGEX_DENY)
|
||||
#define PERF_STAT_COUNT 11
|
||||
|
||||
// How long to wait before retrying a MAC lookup that came back empty. Long
|
||||
// enough that a busy client costs one netlink round trip a minute instead of
|
||||
// one per query, short enough that a genuinely late ARP entry is still picked
|
||||
// up quickly.
|
||||
#define MAC_LOOKUP_BACKOFF 60
|
||||
|
||||
// Monotonic seconds for the backoff deadline. A wall-clock stamp would let an
|
||||
// NTP or admin clock step suppress MAC lookups for the size of the step (or
|
||||
// expire them early), which is why the encrypted-DNS listeners use the same
|
||||
// clock for their deadlines.
|
||||
static inline uint32_t mac_lookup_now(void)
|
||||
{
|
||||
struct timespec ts;
|
||||
clock_gettime(CLOCK_MONOTONIC, &ts);
|
||||
return (uint32_t)ts.tv_sec;
|
||||
}
|
||||
|
||||
static struct {
|
||||
uint64_t calls; // number of invocations
|
||||
uint64_t total_us; // cumulative microseconds
|
||||
@@ -1226,7 +1243,20 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
|
||||
// Don't do this for internally generated queries (e.g., DNSSEC), if the
|
||||
// MAC address is already known or if the netlink socket is not available
|
||||
// (e.g., when retrying a query using TCP after UDP truncation)
|
||||
if(!internal_query && client->hwlen < 1 && daemon->netlinkfd > 0)
|
||||
//
|
||||
// A client that has no ARP entry never gets one from this lookup, and the
|
||||
// unresolved hwlen then re-triggers it on every single query. Measured on a
|
||||
// loopback client: find_mac() itself costs ~67 us and, because the SHM lock
|
||||
// is dropped around it, re-acquiring the lock costs a further ~108 us - some
|
||||
// 27% of FTL's per-query work, spent re-learning that the answer is still no.
|
||||
// Back off after a miss instead; a MAC that appears later is picked up on the
|
||||
// next attempt.
|
||||
// A loopback client is a special case of the same waste: 127.0.0.0/8 and ::1
|
||||
// never appear in the neighbour table at all, so unlike a client that might
|
||||
// gain an ARP entry later there is nothing to come back for. Skip it outright
|
||||
// rather than retrying once per backoff interval forever.
|
||||
if(!internal_query && !mysockaddr_is_loopback(addr) && client->hwlen < 1 &&
|
||||
daemon->netlinkfd > 0 && mac_lookup_now() >= client->hwaddr_next_try)
|
||||
{
|
||||
// find_mac() may trigger a netlink kernel call
|
||||
// (iface_enumerate) to refresh the ARP table on a cache miss.
|
||||
@@ -1251,6 +1281,9 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
|
||||
client->flags.found_group = false;
|
||||
memcpy(client->hwaddr, hwaddr, sizeof(hwaddr));
|
||||
client->hwlen = hwlen;
|
||||
// Nothing found: do not ask again for a while.
|
||||
if(hwlen < 1)
|
||||
client->hwaddr_next_try = mac_lookup_now() + MAC_LOOKUP_BACKOFF;
|
||||
}
|
||||
|
||||
// Re-fetch all SHM pointers as SHM may have been remapped
|
||||
|
||||
+2
-1
@@ -36,7 +36,8 @@
|
||||
#include "lookup-table.h"
|
||||
|
||||
/// The version of shared memory used
|
||||
#define SHARED_MEMORY_VERSION 17
|
||||
// 18: clientsData gained hwaddr_next_try (MAC-lookup backoff)
|
||||
#define SHARED_MEMORY_VERSION 18
|
||||
|
||||
/// The name of the shared memory. Use this when connecting to the shared memory.
|
||||
#define SHMEM_PATH "/dev/shm"
|
||||
|
||||
Reference in New Issue
Block a user