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

RFC: interruptible POSIX locks #12

Open
pwmarcz opened this issue Jul 7, 2021 · 6 comments
Open

RFC: interruptible POSIX locks #12

pwmarcz opened this issue Jul 7, 2021 · 6 comments
Assignees
Labels
enhancement New feature or request P: 2

Comments

@pwmarcz
Copy link
Contributor

pwmarcz commented Jul 7, 2021

I'm trying to fix recently added POSIX locks (gramineproject/graphene#2481) so that you can interrupt them, but I ran into a problem trying to do it with current IPC. I don't want to jump and change too much before I learn your opinion.

@boryspoplawski @mkow @dimakuv

The problem

Graphene's current implementation of POSIX locks (fcntl) do not support interrupting the request:

void handler(int signum) {
    printf("got signal %d\n", signum);
}
signal(SIGALRM, &handler);

fcntl(fd, F_SETLK, ...);
pid_t pid = fork();
if (pid == 0) {
    // child process:

    // trigger SIGALRM after 10 seconds
    alarm(10);

    // this should be interrupted:
    fcntl(fd, F_SETLKW, ...);
    ...
}

The fcntl(F_SETLKW) in the child process should wait for 10 seconds and then fail with EINTR (after the process receives SIGALRM). Under Graphene, it hangs indefinitely.

So far, I've seen this checked by LTP (test fcntl16), and I'm pretty sure it's used by stress-ng (see gramineproject/graphene#2510). I'm not sure how much it's used in actual applications, but interrupting the fcntl call seems to be the only sensible way of trying to take a lock with timeout.

How to interrupt an operation over IPC?

Interrupting the lock operation is easy in the single-process version. However, if we are in a remote process, the operation is requested over IPC, and it's not that easy to achieve the same semantics.

The current IPC implementation does something like this:

lock_msg = { POSIX_LOCK_SET, .wait = true, ... };

// this waits for response, and retries on EINTR
ipc_send_message_and_get_response(lock_msg, &result);
return result;

As long as ipc_send_message_and_get_response keeps retrying, we have no way to return earlier. I've been thinking about some solutions:

  1. Stop waiting?: You could be tempted to modify the ipc_send_message_and_get_response() function to not wait (perhaps with some retry = false parameter). However, this does not actually cancel the operation, so the lock will be taken eventually. This is wrong - we want the F_SETLKW operation to fail without side effects.

  2. Stop waiting and issue another call? We could send another IPC message to cancel the pending operation. Keep in mind that the operation might get finished before the "cancel" message is delivered, so we need to learn what the actual result is.

    lock_msg = { POSIX_LOCK_SET, .wait = true, ... };
    
    // this returns -EINTR if the wait is interrupted
    ret = ipc_send_message_and_get_response(lock_msg, /*retry=*/false, &result);
    
    if (ret == -EINTR) {
        // Cancel, and get result. `result` will be -EINTR if we succesfully canceled the operation, 
        // or something else (0, -ENOMEM, ...) if the operation finished in the meantime
        cancel_msg = { POSIX_LOCK_CANCEL, msg.seq, ... };
        ipc_send_message_and_get_response(cancel_msg, /*retry=*/true, &result);
    }
    return result;

    The problem with that is that if the operation does get finished in the meantime, the remote side has to keep storing the result in case we receive POSIX_LOCK_CANCEL. So we would need some way of cleaning up that result.

    Also, if the response to POSIX_LOCK_SET is sent in the meantime, we are going to just drop it.

  3. Keep waiting for the original response? Ideally, we want to always get the response for the original message, but sometimes we want to trigger the response to come early. So we could register a wait, but on EINTR, send additional message in the meantime, then return to the previous wait:

    lock_msg = { POSIX_LOCK_SET, .wait = true, ... };
    
    ipc_send_message_and_register_wait(lock_msg);
    ret = wait_for_response(lock_msg, /*retry=*/false);
    if (ret == -EINTR) {
         // Cancel. That will cause response to `lock_msg` to be sent without further delay
         // (if it hasn't been sent already).
         cancel_msg = { POSIX_LOCK_CANCEL, msg.seq, ... };
         ipc_send_message(cancel_msg);
    
         // Wait for response to the original message. `result` will be -EINTR if we canceled the operation,
         // or something else (0, -ENOMEM...) if the operation finished in the meantime.
         wait_for_response(lock_msg, /*retry=*/true, &result);
    }
    return result;

    I like that, but it makes the IPC API more complicated: instead of one function that does everything, there are separate stages (first send a message and register a wait, then wait for response). That means managing some state between calls (a "waiter" registered for the IPC worker) that currently is local to ipc_send_message_and_get_response.

  4. Add an on_interrupt callback? The above could be simplified by keeping the ipc_send_message_and_get_response as is, but making it perform some additional action when the wait is interrupted.

    int send_cancel(struct shim_ipc_message* msg) {
         cancel_msg = { POSIX_LOCK_CANCEL, msg->seq, ... };
         return ipc_send_message(cancel_msg);        
    }
    
    lock_msg = { POSIX_LOCK_SET, .wait = true, ... };
    
    // this calls `on_interrupt` when the wait is interrupted, then keeps on waiting
    // (or fails, if `on_interrupt` returned failure)
    ipc_send_message_and_get_response(lock_msg, /*on_interrupt=*/&send_cancel, &result);
    return result;

    This would certainly be easiest to use, but it's a pretty specific use-case. I think it's worth it only if we expect to use it again soon.

So I think either 3 or 4 are viable. But maybe I'm missing something and there is an easier way?

@dimakuv
Copy link

dimakuv commented Jul 8, 2021

I prefer option 4.

Option 3 sounds like an overkill to fix this issue (and similar "amend/cancel pending operation" issues). Splitting our nice ipc_send_message_and_get_response() into two phases, with some state maintained in-between, sounds complex.

By the way, what is the struct shim_ipc_message* msg in your send_cancel callback? Why is this argument needed? And what is msg->seq, how does it help?

@pwmarcz
Copy link
Contributor Author

pwmarcz commented Jul 8, 2021

Option 3 sounds like an overkill to fix this issue (and similar "amend/cancel pending operation" issues). Splitting our nice ipc_send_message_and_get_response() into two phases, with some state maintained in-between, sounds complex.

I agree; on the other hand, option 4 sounds over-specific. But since it seems cheap, maybe it doesn't hurt to try it out. I'll try finishing my branch with option 4 and we'll see how it looks.

By the way, what is the struct shim_ipc_message* msg in your send_cancel callback? Why is this argument needed? And what is msg->seq, how does it help?

It was supposed to be the original message (lock_msg). I need seq which is a message identifier (currently added by ipc_send_message_and_get_response), so that the "cancel" message says which operation to cancel. Actually, I'd probably like to use a generic void* arg parameter in the callback instead, to make it a bit more flexible; then lock_msg will be passed explicitly.

@mkow
Copy link
Member

mkow commented Jul 11, 2021

How does 4. work with both messages? Will we wait for responses for both? (original lock + cancel msg)
Overall it seems like an ok solution for now.

@boryspoplawski
Copy link
Contributor

boryspoplawski commented Jul 11, 2021

  1. seq wasn't mean to be a public interface, it was intended to stay inside IPC implementation (i.e. not meant to be used as some other objects ID, which I guess 4 proposes - use seq as lock request ID). Using it outside and sending two messages with same seq may break some core assumptions.
  2. ipc_send_message_and_get_response is not interruptible by design. It's meant to be an internal mechanism and all public IPC interfaces are meant to be building blocks for other subsystems. Response messages are supposed to come ~soon, i.e. they are not supposed to do sleeps/waits in the message recipient (destination IPC worker).
    Also doing sleeps with functions other than thread_wait is not signal-safe/fully-signal-aware.

I would say you cannot get away from using something like:

struct X x;
add_to_global_list(&x);
ipc_send_msg(get_lock);
thread_wait();
if (!check_if_condition_happend(&x, is_lock_taken, atomic_wrt_callbacks=True)) {
    ipc_send_msg(cancel_get_lock);
}
remove_from_global_list(&x);
return check_if_condition_happend(&x, is_lock_taken, atomic_wrt_callbacks=False);

and have some callbacks on lock_get response messages. This might sound a bit like reimplementing responses, but this seems to be a pretty special case.
Also this sounds like something that might need to be built-in sync server/client code?

@pwmarcz
Copy link
Contributor Author

pwmarcz commented Jul 19, 2021

OK, looks like no easy win here. On the other hand, reimplementing responses doesn't seem so bad: an IPC abstraction for "long-term operations" might be exactly what I need. I'll think about it more.

@boryspoplawski
Copy link
Contributor

@pwmarcz It's quite possible that we need to add some more advanced IPC primitives than we have right now.

@mkow mkow transferred this issue from gramineproject/graphene Sep 9, 2021
@dimakuv dimakuv added enhancement New feature or request P: 2 labels Nov 25, 2021
kailun-qin pushed a commit to kailun-qin/gramine that referenced this issue Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P: 2
Projects
None yet
Development

No branches or pull requests

4 participants