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

Fix for Query Log filtering and memory optimizations #1032

Merged
merged 4 commits into from Jan 18, 2021

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jan 18, 2021

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


This PR fixes #1026 and https://github.com/pi-hole/FTL/issues/1027 by ensuring the first upstream destination is not a collector for all kinds of other queries. There was an ill assumption in the API code leading to a (much) too loose filtering.

Furthermore, I added a bitfield to the queriesData struct to allow further simplification of the API code easing future maintainability (the same code will go into v6.0 so this still matters). While at it, I added a mechanism to ensure the size of memory objects does not change without us noticing (the CI will fail).

The first commit optimized the internal datastructure FTL uses. By this, we were able to save 8 bytes of memory per query, oer client and per regex. On a typical setup with some clients, some regex and about 25,000 queries per day, this means a memory usage reduction of roughly 0.2 MB. This continues our efforts to fit Pi-hole into the smallest possible memory footprint (v5.0 already drastically reduced memory footprint due to using a B-tree for the blocking domains).

…inimize padding). This reduces the size of query, client, and regex records by 8 bytes per item. Note that this optimization was done on x86_64 and may not apply for other architectures (32bit architectures already used less padding).

Signed-off-by: DL6ER <dl6er@dl6er.de>
…rom increasing the memory needs unintentionally (e.g. due to sub-optimal padding)

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
… default to differentiate easily what was forwarded (ID will be >= 0) and what not (ID == -1). Store the upstream server also for other query types that were forwarded (like queries blocked during CNAME inspection).

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER marked this pull request as ready for review January 18, 2021 20:15
@DL6ER
Copy link
Member Author

DL6ER commented Jan 18, 2021

Confirmed working by the original reporter of the issue (#1026)

@dschaper dschaper added the PR: Approved Open Pull Request, Approved by required number of reviewers label Jan 18, 2021
@DL6ER DL6ER merged commit 4d83a5a into development Jan 18, 2021
@DL6ER DL6ER deleted the fix/querylog_filtering branch January 18, 2021 20:58
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/new-update-ftl-5-5-fatal-trying-to-access-upstream-id-1-but-maximum-is-1024-pihole-ftl-log-errors/43177/4

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/new-update-ftl-5-5-fatal-trying-to-access-upstream-id-1-but-maximum-is-1024-pihole-ftl-log-errors/43177/16

@dschaper
Copy link
Member

DL, where did the object sizes for the assertions come from?

There's a report on https://discourse.pi-hole.net/t/static-assert-h-error-compiling-from-source-alpine-linux/54915 that fails to build on an armv7 with different object sizes.

For documentation:

[ 79%] Building C object src/database/CMakeFiles/database.dir/common.c.o
In file included from /home/user/FTL/src/database/../static_assert.h:11,
                 from /home/user/FTL/src/database/../datastructure.h:19,
                 from /home/user/FTL/src/database/../shmem.h:19,
                 from /home/user/FTL/src/database/common.c:15:
/home/user/FTL/src/database/../static_assert.h:15:3: error: static assertion failed: "Expected size of queriesData is 44 on this architecture."
   15 |   static_assert(sizeof(OBJECT) == EXPECTED , "Expected size of " #OBJECT " is " #EXPECTED " on this architecture.");
      |   ^~~~~~~~~~~~~
/home/user/FTL/src/database/../static_assert.h:26:9: note: in expansion of macro 'STATIC_ASSERT'
   26 |         STATIC_ASSERT(OBJECT, SIZEARM)
      |         ^~~~~~~~~~~~~
/home/user/FTL/src/database/../datastructure.h:55:1: note: in expansion of macro 'ASSERT_SIZEOF'
   55 | ASSERT_SIZEOF(queriesData, 56, 44, 44);
      | ^~~~~~~~~~~~~
/home/user/FTL/src/database/../static_assert.h:15:3: error: static assertion failed: "Expected size of upstreamsData is 624 on this architecture."
   15 |   static_assert(sizeof(OBJECT) == EXPECTED , "Expected size of " #OBJECT " is " #EXPECTED " on this architecture.");
      |   ^~~~~~~~~~~~~
/home/user/FTL/src/database/../static_assert.h:26:9: note: in expansion of macro 'STATIC_ASSERT'
   26 |         STATIC_ASSERT(OBJECT, SIZEARM)
      |         ^~~~~~~~~~~~~
/home/user/FTL/src/database/../datastructure.h:67:1: note: in expansion of macro 'ASSERT_SIZEOF'
   67 | ASSERT_SIZEOF(upstreamsData, 640, 624, 624);
      | ^~~~~~~~~~~~~
/home/user/FTL/src/database/../static_assert.h:15:3: error: static assertion failed: "Expected size of clientsData is 668 on this architecture."
   15 |   static_assert(sizeof(OBJECT) == EXPECTED , "Expected size of " #OBJECT " is " #EXPECTED " on this architecture.");
      |   ^~~~~~~~~~~~~
/home/user/FTL/src/database/../static_assert.h:26:9: note: in expansion of macro 'STATIC_ASSERT'
   26 |         STATIC_ASSERT(OBJECT, SIZEARM)
      |         ^~~~~~~~~~~~~
/home/user/FTL/src/database/../datastructure.h:94:1: note: in expansion of macro 'ASSERT_SIZEOF'
   94 | ASSERT_SIZEOF(clientsData, 696, 668, 668);
      | ^~~~~~~~~~~~~
In file included from /home/user/FTL/src/database/../static_assert.h:11,
                 from /home/user/FTL/src/database/../config.h:21,
                 from /home/user/FTL/src/database/common.c:17:
/home/user/FTL/src/database/../static_assert.h:15:3: error: static assertion failed: "Expected size of ConfigStruct is 104 on this architecture."
   15 |   static_assert(sizeof(OBJECT) == EXPECTED , "Expected size of " #OBJECT " is " #EXPECTED " on this architecture.");
      |   ^~~~~~~~~~~~~
/home/user/FTL/src/database/../static_assert.h:26:9: note: in expansion of macro 'STATIC_ASSERT'
   26 |         STATIC_ASSERT(OBJECT, SIZEARM)
      |         ^~~~~~~~~~~~~
/home/user/FTL/src/database/../config.h:94:1: note: in expansion of macro 'ASSERT_SIZEOF'
   94 | ASSERT_SIZEOF(ConfigStruct, 112, 104, 104);
      | ^~~~~~~~~~~~~
make[2]: *** [src/database/CMakeFiles/database.dir/build.make:76: src/database/CMakeFiles/database.dir/common.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:348: src/database/CMakeFiles/database.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 79%] Built target sqlite3
make: *** [Makefile:136: all] Error 2

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/static-assert-h-error-compiling-from-source-alpine-linux/54915/7



#define STATIC_ASSERT(OBJECT, EXPECTED) \
static_assert(sizeof(OBJECT) == EXPECTED , "Expected size of " #OBJECT " is " #EXPECTED " on this architecture.");
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to have the sizeof(OBJECT) included in the warning message? It only tells me what the expected size is, I don't know what the actual size was.

@DL6ER
Copy link
Member Author

DL6ER commented Apr 18, 2022

It isn't possible to print the actual size in the warning due to preprocessor limitations. The numbers are inferred from research with a modified FTL version that printed it. While this has served us well for a long time, PR #1329 will make this more flexible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Code maintanance PR: Approved Open Pull Request, Approved by required number of reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants