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.

Sign up for GitHub

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

Jump to bottom

nanocoap: cache for CoAP responses #13889

Merged
merged 3 commits into from
May 10, 2022
Merged

nanocoap: cache for CoAP responses #13889

merged 3 commits into from
May 10, 2022

Conversation

cgundogan
Copy link
Member

@cgundogan cgundogan commented Apr 17, 2020 •
edited
Loading

Contribution description

This PR adds a cache for CoAP response messages to nanocoap. RFC7252, Section-5.6 elaborates on the internals of a cache implementation.

The basic idea is to reduce response time and enhance bandwidth usage by storing CoAP responses at intermediary proxy nodes. The cache key is calculated using the information in RFC7252, Section 5.10. Optimized cache key calculations based on other mechanisms (e.g., using the ETAG) are not included in this PR (and firstly require a proper cache<->proxy integration).

This proposed implementation uses a simple LRU strategy to replace cache entries as soon as the available cache space is used up. The replacement strategy is passed as a function pointer, so that we can easily add more sophisticated strategies in the future, if needed.

The demonstrated integration into gcoap is preliminary .. depending on the direction we take in #13790, the included changes to gcoap may change.
The cache implementation now integrates into the forward proxy in #13790.

Testing procedure

  1. unittests are available that show the interaction with the cache
  2. To test manually on native:
    1. build two gcoap examples, where one of them uses USEMODULE=nanocoap_cache
    2. on the native instance that does not include the nanocoap_cache module, type
     coap get fe80::bc63:a1ff:fe23:df98 5683 /cli/stats
    the result should be 0. The caching node (our fictitious proxy) caches the generated response.
    3. on the same node, type 
    coap put fe80::bc63:a1ff:fe23:df98 5683 /cli/stats 1
coap get fe80::bc63:a1ff:fe23:df98 5683 /cli/stats
    The result should still be 0, because the caching node does not invoke the request handler function, but performs a cache lookup.
    4. After 60 seconds, repeat the above get request, and the response should now contain 1.

The default max-age (see RFC7252, Section 5.10.5) is 60 seconds, if no explicit max-age options are used.

Issues/PRs references

depends on #13893
depends #13790

@cgundogan cgundogan added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: CoAP Area: Constrained Application Protocol implementations labels Apr 17, 2020
@cgundogan cgundogan added this to the Release 2020.07 milestone Apr 17, 2020
@cgundogan cgundogan requested a review from kb2ma April 17, 2020 12:34
@cgundogan cgundogan mentioned this pull request Apr 17, 2020
Merged
@cgundogan
Copy link
Member Author

rebased to master and dropped the commit that was merged in #13893

@kb2ma
Copy link
Member

kb2ma commented Apr 23, 2020

This PR seems like a logical extension to #13790. I'll take a closer look after I finish with the follow-up for the payload helper PR.

@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 24, 2020
@cgundogan
Copy link
Member Author

The cache implementation now integrates into the forward proxy code back in #13790, instead of in gcoap.c.

@cgundogan cgundogan requested a review from jia200x as a code owner May 15, 2020 09:04
@cgundogan
Copy link
Member Author

the last two commits add kconfig configurations for cache related parameters

@benpicco benpicco requested a review from chrysn October 21, 2020 22:20
@kaspar030 kaspar030 removed this from the Release 2021.04 milestone Apr 23, 2021
@cgundogan cgundogan requested a review from MrKevinWeiss as a code owner April 27, 2022 13:18
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Apr 27, 2022
miri64
miri64 previously approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Please squash!

@cgundogan cgundogan removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 28, 2022
@miri64 miri64 dismissed their stale review April 28, 2022 10:42

Seems @cgundogan still has some cleaning up to do, that I missed.

miri64
miri64 reviewed Apr 29, 2022
View reviewed changes
tests/unittests/tests-nanocoap_cache/tests-nanocoap_cache.c Outdated

/* the absolute time of max-age should be at approx. now + 30 sec
(1 sec buffer) */
now = xtimer_now_usec();
Copy link
Member

Choose a reason for hiding this comment

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

Here and below (and the unittest's Makefile)

Suggested change
now = xtimer_now_usec();
now = ztimer_now(ZTIMER_SEC);

no? There is no relation between xtimer_now_usec() (or ztimer_now(ZTIMER_USEC) for that matter) to ztimer_now(ZTIMER_SEC) and the latter is used internally to the cache... so comparing two timers who have no common time basis, is non-sensical in the tests (and might even lead to spurious failures).

Copy link
Member

Choose a reason for hiding this comment

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

See also https://github.com/RIOT-OS/RIOT/pull/13889/files#r827160244

@cgundogan cgundogan force-pushed the pr/coca branch 3 times, most recently from 2282ee9 to fdafd37 Compare May 10, 2022 10:02
@miri64
Copy link
Member

miri64 commented May 10, 2022

Please squash.

@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 10, 2022
miri64
miri64 approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Comments are now addressed. ACK!

@miri64 miri64 enabled auto-merge May 10, 2022 14:07
@miri64 miri64 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 10, 2022
@miri64 miri64 removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label May 10, 2022
@miri64
Copy link
Member

miri64 commented May 10, 2022

Except for some unrelated errors (https://ci.riot-os.org/RIOT-OS/RIOT/13889/fee3320023b5973c570de1cf64bc6ff2da82bd17/output/run_test/tests/thread_float/samr21-xpro:gnu.txt, https://ci.riot-os.org/RIOT-OS/RIOT/13889/fee3320023b5973c570de1cf64bc6ff2da82bd17/output/run_test/tests/sys_sched_round_robin/esp32-wroom-32:gnu.txt, https://ci.riot-os.org/RIOT-OS/RIOT/13889/fee3320023b5973c570de1cf64bc6ff2da82bd17/output/run_test/tests/periph_timer/esp32-wroom-32:gnu.txt), there were some MSP430 compile errors. Please fix.

@miri64
Copy link
Member

miri64 commented May 10, 2022

@cgundogan was faster ^^

@cgundogan
Copy link
Member Author

Amended a fix for a minor signedness issue. Re-running murdock.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 10, 2022
@miri64 miri64 merged commit 02fb040 into RIOT-OS:master May 10, 2022
@cgundogan cgundogan deleted the pr/coca branch May 11, 2022 07:17
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas left review comments

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@kaspar030 kaspar030 kaspar030 left review comments

@benpicco benpicco benpicco left review comments

@kfessel kfessel kfessel left review comments

@miri64 miri64 miri64 approved these changes

@kb2ma kb2ma Awaiting requested review from kb2ma

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@chrysn chrysn Awaiting requested review from chrysn

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.