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

Enhanced Timer Handling and Testing for One Shot Timer #2562

Open
wants to merge 2 commits into
base: criu-dev
Choose a base branch
from
Open

Enhanced Timer Handling and Testing for One Shot Timer #2562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3249a83
Update timer handling to properly respect the it_value field
hckuo Jan 7, 2025
c20a4cf
test: zdtm: Enhance timers test with multiple timer configurations
hckuo Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion criu/pie/restorer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2210,7 +2210,7 @@ __visible long __export_restore_task(struct task_restore_args *args)
* code below doesn't fail due to bad timing values.
*/

#define itimer_armed(args, i) (args->itimers[i].it_interval.tv_sec || args->itimers[i].it_interval.tv_usec)
#define itimer_armed(args, i) (args->itimers[i].it_interval.tv_sec || args->itimers[i].it_interval.tv_usec || args->itimers[i].it_value.tv_sec || args->itimers[i].it_value.tv_usec)
Copy link
Member

Choose a reason for hiding this comment

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

nit: The limit on the length of lines is 80 columns and this is a strongly preferred limit.

Copy link
Member

Choose a reason for hiding this comment

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

actually, you need to check just it_value.tv_sec and it_value.tv_usec:

#define itimer_armed(args, i) (args->itimers[i].it_value.tv_sec || args->itimers[i].it_value.tv_usec)

If this two fields are zero, setitimer disarms the timer.


if (itimer_armed(args, 0))
sys_setitimer(ITIMER_REAL, &args->itimers[0], NULL);
Expand Down
15 changes: 3 additions & 12 deletions criu/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ static inline int timeval_valid(struct timeval *tv)

static inline int decode_itimer(char *n, ItimerEntry *ie, struct itimerval *val)
{
if (ie->isec == 0 && ie->iusec == 0) {
if (ie->isec == 0 && ie->iusec == 0 && ie->vsec == 0 && ie->vusec == 0) {
memzero_p(val);
return 0;
}
Expand All @@ -29,17 +29,8 @@ static inline int decode_itimer(char *n, ItimerEntry *ie, struct itimerval *val)
return -1;
}

if (ie->vsec == 0 && ie->vusec == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This code handles the case when the timer is armed, but it_value is set to 0:
https://man7.org/linux/man-pages/man2/setitimer.2.html

 If  both fields of it_value are zero, then this timer is currently disarmed (inactive).

/*
* Remaining time was too short. Set it to
* interval to make the timer armed and work.
*/
val->it_value.tv_sec = ie->isec;
val->it_value.tv_usec = ie->iusec;
} else {
val->it_value.tv_sec = ie->vsec;
val->it_value.tv_usec = ie->vusec;
}
val->it_value.tv_sec = ie->vsec;
val->it_value.tv_usec = ie->vusec;

if (!timeval_valid(&val->it_value)) {
pr_err("Invalid timer value\n");
Expand Down
32 changes: 21 additions & 11 deletions test/zdtm/static/timers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,17 @@ static void timer_tick(int sig)
}
}

static void setup_timers(void)
static void setup_timers(const struct itimerval *tv)
{
int i;
struct itimerval tv = {
.it_interval = { .tv_sec = 0, .tv_usec = 100000 },
.it_value = { .tv_sec = 0, .tv_usec = 100 },
};

for (i = 0; i < NUM_TIMERS; i++) {
if (signal(timer_tests[i].signal, timer_tick) == SIG_ERR) {
pr_perror("can't set signal handler %d", i);
exit(1);
}

if (setitimer(timer_tests[i].timer_type, &tv, NULL) < 0) {
if (setitimer(timer_tests[i].timer_type, tv, NULL) < 0) {
pr_perror("can't set timer %d", i);
exit(1);
}
Expand Down Expand Up @@ -77,13 +73,27 @@ static void check_timers(void)

int main(int argc, char **argv)
{
test_init(argc, argv);
const struct itimerval tv = {
.it_interval = { .tv_sec = 0, .tv_usec = 100000 },
.it_value = { .tv_sec = 0, .tv_usec = 100 },
};
const struct itimerval oneshot_tv = {
.it_interval = { .tv_sec = 0, .tv_usec = 0 },
.it_value = { .tv_sec = 1, .tv_usec = 100 },
};
const struct itimerval tvs[2] = {
tv,
oneshot_tv,
};

setup_timers();
test_init(argc, argv);
for (int i = 0; i < 2; i++) {
setup_timers(&tvs[i]);

test_daemon();
test_waitsig();
test_daemon();
test_waitsig();
Copy link
Member

Choose a reason for hiding this comment

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

It will not work. test_waitsig does its job when it is called first time. If you call it second time, it returns immediately:

criu/test/zdtm/lib/test.c

Line 365 in 27a5b9a

futex_wait_while(&sig_received, 0);

I suggest to add a new test. Here is my version of it: avagin@c02e313

It fails without the your fix and it passes with it:


# ./test/zdtm.py run -t zdtm/static/timers01 --ignore-taint
userns is supported
The kernel is tainted: '12352'
=== Run 1/1 ================ zdtm/static/timers01
======================= Run zdtm/static/timers01 in uns ========================
Start test
./timers01 --pidfile=timers01.pid --outfile=timers01.out
Run criu dump
Run criu restore
=[log]=> dump/zdtm/static/timers01/64/1/restore.log
------------------------ grep Error ------------------------
b'(00.009848)      1: No ipcns-sem-11.img image'
b'(00.018559)      1: net: Try to restore a link 10:1:lo'
b'(00.018566)      1: net: Restoring link lo type 1'
b'(00.021284)      1: net: \tRunning ip addr restore'
b'Error: ipv4: Address already assigned.'
b'Error: ipv6: address already assigned.'
------------------------ ERROR OVER ------------------------
Send the 15 signal to  92
Wait for zdtm/static/timers01(92) to die for 0.100000
################ Test zdtm/static/timers01 FAIL at result check ################
Test output: ================================
17:44:11.873:     4: FAIL: timers01.c:54: 0 isn't in [3600, 3300] (errno = 11 (Resource temporarily unavailable))

 <<< ================================
##################################### FAIL #####################################


check_timers();
check_timers();
}
return 0;
}
Loading