Skip to content

Add the utility function to clear page cache #741

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

Open
wants to merge 16 commits into
base: branch-25.08
Choose a base branch
from

Conversation

kingcrimsontianyu
Copy link
Contributor

@kingcrimsontianyu kingcrimsontianyu commented Jun 1, 2025

This PR introduces a utility function to clear page cache in C++ and Python.

@kingcrimsontianyu kingcrimsontianyu added feature request New feature or request non-breaking Introduces a non-breaking change python Affects the Python API of KvikIO c++ Affects the C++ API of KvikIO DO NOT MERGE labels Jun 1, 2025
Copy link

copy-pr-bot bot commented Jun 1, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@kingcrimsontianyu
Copy link
Contributor Author

/ok to test 7051979

@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review June 4, 2025 13:28
@kingcrimsontianyu kingcrimsontianyu requested review from a team as code owners June 4, 2025 13:28
if (clear_dirty_pages) { sync(); }
std::string param = reclaim_dentries_and_inodes ? "3" : "1";
std::stringstream ss;
ss << "echo " << param << " | sudo tee /proc/sys/vm/drop_caches 1>/dev/null";
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@kingcrimsontianyu kingcrimsontianyu Jun 4, 2025

Choose a reason for hiding this comment

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

I think cuDF's implementation still relies on sudo or superuser privilege. The logic is:

  1. run the cache clearing command in a shell
  2. run the sudo-ed command in a shell
  3. if both of them fail, throw an exception

Consequently:

  • General case
    • For superuser, 1 succeeds.
    • For non-superuser, 1 fails, 2 succeeds once providing the right password.
  • Docker container
    • Without --priviledged argument, both fail regardless of superuser or not.
    • With --priviledged, the same with the general case (except that no password prompt for non-superuser).

So this PR simplifies 1 and 2 to a single sudo command, and also returns boolean to indicate success/failure instead of throwing on failure.

PS: I actually had another implementation earlier that does not use sudo at all: 606b233#diff-e00c8157cb029cd5327cfe9b8b834601b40dcfef0b847bc3d6272e3c7b5c3c1cR216
This method does not have the problem in system() or popen() (they create a separate process and run the shell command, which adds to security vulnerability). But it also lacks the convenience: users need to run the entire process with sudo in order to clear the cache, as opposed to only sudo the cache clearing part in the shell process.

PSS: I also had an experimental snippet that does something like below, with the hope that I can achieve temporary privilege elevation for non-superusers:

perform tasks
{
    // temporarily elevate privilege
    // stash the real uid and real gid
    SuperUserRaiiContext ctx;
    perform tasks
} //  set to the original real uid and real gid
perform tasks

But I scratched this idea too, because it still requires users to run the process with elevated privilege, and as a result the code prior to SuperUserRaiiContext is still privileged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vuule explained that the non-sudo command is used on specially configured machines where unprivileged users have no access to sudo executable but can run sysctl. This complicates things, as the sysctl command does not return non-zero code on insufficient permission failure, unlike the other approach echo 3 | tee /proc/sys/vm/drop_caches which does. So I changed to cuDF's implementation that uses stderr to check failure instead.

@kingcrimsontianyu kingcrimsontianyu requested a review from vuule June 4, 2025 14:40
@@ -209,4 +212,16 @@ std::pair<std::size_t, std::size_t> get_page_cache_info(int fd)
SYSCALL_CHECK(munmap(addr, file_size));
return {num_pages_in_page_cache, num_pages};
}

bool clear_page_cache(bool reclaim_dentries_and_inodes, bool clear_dirty_pages)
Copy link
Contributor

Choose a reason for hiding this comment

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

when would we want to call this without clearing the dirty pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say these options are for curious minds who wonder the respective effect on page cache 😃

from kvikio.remote_file import RemoteFile, is_remote_file_available

__all__ = [
"__git_commit__",
"__version__",
"CuFile",
"get_page_cache_info",
"clear_page_cache",
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically we try to keep __all__ alphabetized. Could you sort these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Sorted.

@kingcrimsontianyu kingcrimsontianyu requested a review from madsbk June 5, 2025 19:35
Comment on lines 195 to 196
* @note This function creates a child process and executes the cache clearing shell command with
* `sudo`. Superuser privilege is therefore generally needed for the function to return `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Update to say it first try without sudo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. The comments have been updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Affects the C++ API of KvikIO feature request New feature or request non-breaking Introduces a non-breaking change python Affects the Python API of KvikIO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants