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

Optimize timestamps with per-CPU caching #2349

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MorganaFuture
Copy link
Contributor

@MorganaFuture MorganaFuture commented Mar 5, 2025

What

Implement per-CPU cached timestamp mechanism for tfw_current_timestamp()
Add automatic refresh when cached timestamp becomes stale
Only cache timestamps in softirq context

Why

ktime_get_real_ts64() is a heavyweight kernel function

Links
2275

@krizhanovsky
Copy link
Contributor

@MorganaFuture we'be been lacking resources to implement this feature, so thank you!

This is draft, we shouldn't review it yet, right?

@MorganaFuture
Copy link
Contributor Author

This PR is not ready yet. I'm having some difficulties running tests for Tempesta. Once I make sure everything works as expected, I will update the PR status

@keshonok
Copy link
Contributor

BTW, why not simply update a per-CPU variable in a timer SoftIRQ? Then each tfw_current_timestamp() call would be a simple value retrieval which is as lightweight as it comes.

@MorganaFuture MorganaFuture force-pushed the morganaFuture/timestamp-optimization branch from cf97fdd to 81600a9 Compare March 12, 2025 12:40
@MorganaFuture MorganaFuture requested a review from keshonok March 12, 2025 12:45
@MorganaFuture MorganaFuture marked this pull request as ready for review March 12, 2025 12:46
@MorganaFuture
Copy link
Contributor Author

@keshonok could you please take a look at changes?

@krizhanovsky krizhanovsky requested review from krizhanovsky and const-t and removed request for keshonok March 24, 2025 16:29
@MorganaFuture MorganaFuture force-pushed the morganaFuture/timestamp-optimization branch from dc7f740 to 32f83f5 Compare March 25, 2025 12:15

/* Update the timestamp on all CPUs */
for_each_online_cpu(cpu) {
per_cpu(tfw_ts_cache, cpu) = ts.tv_sec;
Copy link
Contributor

@const-t const-t Mar 25, 2025

Choose a reason for hiding this comment

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

It looks not good, however we do it only once per second. But I have doubts about current solution anyway. Suggested in the task approach seems more elegant. We can conditionally update per-cpu cached time relying on jiffies inside softirq handler. @krizhanovsky What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@const-t What do you think about this solution ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MorganaFuture In this solution we do check/update timer on each call of tfw_current_timestamp, but we can improve it doing check/update only once per softirq handler, and then in tfw_current_timestamp only receive the value. Solution with timer also not bad, but I would prefer to minimize cache bouncing even once per second. However, maybe I'm trying to over optimize, therefore I would like to hear also your @MorganaFuture opinion as well as @krizhanovsky.

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.

4 participants