Skip to content

Add extra statistic to track the number of signature validation operations - #1289

Merged
gthess merged 4 commits into
NLnetLabs:masterfrom
rijswijk:master
Jul 12, 2025
Merged

gthess merged 4 commits into
NLnetLabs:masterfrom
rijswijk:master

Conversation

@rijswijk

Copy link
Copy Markdown
Contributor

This PR adds an extra statistic to the extended statistics to track the number of signature validation operations performed by the validator module. This is mostly useful for research purposes, as it makes it possible to track the workload required for cryptographic operations over time.

The statistic tracks the number of calls to verify_canonrrset(..) in val_sigcrypt.c. The implementation follows the same pattern as for the statistic that tracks the number of bogus RRsets. That is: there is a lock around operations that change or read the statistic (I assume this is because the validator module is not replicated in each thread if Unbound runs multi-threaded).

It would be much appreciated if this patch could be considered for merging as it would help us in research into performance of PQC algorithms; I admit that the extra lock/unlock operation around updates of the statistic adds overhead, I didn't immediately see an easy way to mitigate this as information on whether or not extended statistics are enabled is not available at the abstraction level where the statistic needs to be updated. It seems this is deemed acceptable for tracking the number of bogus RRsets, so hoping one more lock/unlock is fine here.

@gthess gthess self-assigned this Jun 4, 2025
@gthess

gthess commented Jun 4, 2025

Copy link
Copy Markdown
Member

Since this is about every single validation operation I would be concerned about the locking.
I would prefer if the statistic would live/behave similar to ans_bogus

size_t ans_bogus;
where it is stored per thread and later added together when producing the stats.
In dnskey_verify_rrset_sig() you can get access to the mesh_area with qstate->env->mesh.
As for the extended statistics, you can get access to the value via qstate->env->cfg->stat_extended.

@rijswijk

Copy link
Copy Markdown
Contributor Author

Since this is about every single validation operation I would be concerned about the locking. I would prefer if the statistic would live/behave similar to ans_bogus

size_t ans_bogus;

where it is stored per thread and later added together when producing the stats.
In dnskey_verify_rrset_sig() you can get access to the mesh_area with qstate->env->mesh.
As for the extended statistics, you can get access to the value via qstate->env->cfg->stat_extended.

I copied this pattern from how ans_bogus is counted, which also involves a lock it seems, I found this pattern here:

lock_basic_lock(&ve->bogus_lock);

This led me to conclude that it was OK to use locking for this type of statistic, so I'm now somewhat confused as to how ans_bogus is actually getting counted?

@gthess

gthess commented Jun 13, 2025

Copy link
Copy Markdown
Member

You followed the pattern for num_rrset_bogus perfectly :)
(But after this PR I would also like to take the num_rrset_bogus stat outside of the validator module)

You followed the pattern for num_rrset_bogus (lives in the validator module and has to be locked for sharing with threads) but not the pattern for ans_bogus (lives in the mesh area which is per thread, it has to be added together with the rest of the per thread instances when producing the final stats).

(small note that ans_bogus is also counted in the worker for cache replies which do not have a mesh area, but I'll skip mentions to that for clarity, maybe :)

For the ans_bogus that lives in the mesh area:

It is defined here (for stats):

long long ans_bogus;

and here (for the mesh area):
size_t ans_bogus;

It is counted (for the mesh area) here:

m->s.env->mesh->ans_bogus++;

It is gathered here:

s->svr.ans_bogus += (long long)worker->env.mesh->ans_bogus;

You can follow this paradigm and when counting for val_ops in dnskey_verify_rrset_sig() you can reference it through qstate->env->mesh.

@rijswijk

Copy link
Copy Markdown
Contributor Author

Ahh, the naming is confusing, you're (of course) right. Let me see if I can find some time to fix this tomorrow.

@rijswijk

rijswijk commented Jul 6, 2025

Copy link
Copy Markdown
Contributor Author

With thanks to the rain today I finally found time to make the requested change 😅

Comment thread validator/val_sigcrypt.c Outdated
rijswijk and others added 2 commits July 8, 2025 11:41
@gthess

gthess commented Jul 12, 2025

Copy link
Copy Markdown
Member

This looks good to me now, thanks for the fixes!

@gthess
gthess merged commit 44ac818 into NLnetLabs:master Jul 12, 2025
gthess added a commit that referenced this pull request Jul 12, 2025
- Merge #1289 from Roland van Rijswijk-Deij: Add extra statistic to
  track the number of signature validation operations.
  Adds 'num.valops' to extended statistics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants