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

tcmur: improve batch kernel wake up notifications #392

Merged
merged 1 commit into from
Apr 6, 2018
Merged

tcmur: improve batch kernel wake up notifications #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 26 additions & 4 deletions tcmur_aio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void track_aio_request_start(struct tcmur_device *rdev)
pthread_cleanup_pop(0);
}

void track_aio_request_finish(struct tcmur_device *rdev, int *is_idle)
void track_aio_request_finish(struct tcmur_device *rdev, int *wake_up)
{
struct tcmu_track_aio *aio_track = &rdev->track_queue;
pthread_cond_t *cond;
Expand All @@ -62,10 +62,11 @@ void track_aio_request_finish(struct tcmur_device *rdev, int *is_idle)
pthread_mutex_lock(&aio_track->track_lock);

assert(aio_track->tracked_aio_ops > 0);

--aio_track->tracked_aio_ops;
if (is_idle) {
*is_idle = (aio_track->tracked_aio_ops == 0) ? 1 : 0;

if (wake_up) {
++aio_track->pending_wakeups;
*wake_up = (aio_track->pending_wakeups == 1) ? 1 : 0;
}

if (!aio_track->tracked_aio_ops && aio_track->is_empty_cond) {
Expand All @@ -78,6 +79,26 @@ void track_aio_request_finish(struct tcmur_device *rdev, int *is_idle)
pthread_cleanup_pop(0);
}

void track_aio_wakeup_finish(struct tcmur_device *rdev, int *wake_up)
{
struct tcmu_track_aio *aio_track = &rdev->track_queue;

pthread_cleanup_push(_cleanup_mutex_lock, (void *)&aio_track->track_lock);
Copy link

Choose a reason for hiding this comment

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

I don't think cleanup is necessary here. It's necessary when there is a cancellation point in the critical section. My reading of posix (and the linux man page) doesn't include assert as a cancellation point, but I could be wrong... It may output, which could block (and hence should count as a cancellation point), but if it does, it will exit the application anyway so is probably moot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's probably not needed since the daemon will crash if we hit the assert.

I think we are sometimes overly careful. We can do a clean up patch later.

pthread_mutex_lock(&aio_track->track_lock);

if (aio_track->pending_wakeups > 1) {
aio_track->pending_wakeups = 1;
*wake_up = 1;
} else {
assert(aio_track->pending_wakeups > 0);
aio_track->pending_wakeups = 0;
*wake_up = 0;
}

pthread_mutex_unlock(&aio_track->track_lock);
pthread_cleanup_pop(0);
}

static void cleanup_empty_queue_wait(void *arg)
{
struct tcmu_track_aio *aio_track = arg;
Expand Down Expand Up @@ -214,6 +235,7 @@ int setup_aio_tracking(struct tcmur_device *rdev)
int ret;
struct tcmu_track_aio *aio_track = &rdev->track_queue;

aio_track->pending_wakeups = 0;
aio_track->tracked_aio_ops = 0;
ret = pthread_mutex_init(&aio_track->track_lock, NULL);
if (ret != 0) {
Expand Down
2 changes: 2 additions & 0 deletions tcmur_aio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ struct tcmu_device;
struct tcmulib_cmd;

struct tcmu_track_aio {
unsigned int pending_wakeups;
unsigned int tracked_aio_ops;
pthread_mutex_t track_lock;
pthread_cond_t *is_empty_cond;
Expand Down Expand Up @@ -55,6 +56,7 @@ int async_handle_cmd(struct tcmu_device *, struct tcmulib_cmd *,
/* aio request tracking */
void track_aio_request_start(struct tcmur_device *);
void track_aio_request_finish(struct tcmur_device *, int *);
void track_aio_wakeup_finish(struct tcmur_device *, int *);
int aio_wait_for_empty_queue(struct tcmur_device *rdev);

#endif /* __TCMUR_AIO_H */
9 changes: 6 additions & 3 deletions tcmur_cmd_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,15 @@ void tcmur_command_complete(struct tcmu_device *dev, struct tcmulib_cmd *cmd,
static void aio_command_finish(struct tcmu_device *dev, struct tcmulib_cmd *cmd,
int rc)
{
int wakeup;
struct tcmur_device *rdev = tcmu_get_daemon_dev_private(dev);
int wake_up;

track_aio_request_finish(tcmu_get_daemon_dev_private(dev), &wakeup);
tcmur_command_complete(dev, cmd, rc);
if (wakeup)
track_aio_request_finish(rdev, &wake_up);
while (wake_up) {
tcmulib_processing_complete(dev);
track_aio_wakeup_finish(rdev, &wake_up);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could hit a race:

  1. pending_wakeups = 0. thread1 calls track_aio_request_finish, pending_wakeups is incremented to 1, and wake_up=1 is returned. It does it's call to tcmur_command_complete and then falls into the while loop and does tcmulib_processing_complete.

  2. thread2 then calls track_aio_request_finish and increments pending_wakeups=2. wake_up=0 is returned so it is relying on thread1 to call tcmulib_processing_complete.

  3. thread1 then calls track_aio_wakeup_finish. It sees aio_track->pending_wakeups > 1 and sets pending_wakeups=1and returns wake_up=1.

  4. thread1 is super fast and we loop again and makes the tcmulib_processing_complete call and track_aio_wakeup_finish which should have completed the processing for the cmd for thread2. pending_wakeups=1 so it is now set to 0 and wake_up=0 is returned. thread1 is now done. It returns.

  5. thread2 finally calls tcmur_command_complete (or it could have been in the middle of calling it but yet done updating the ring buffer). wake_up was 0 for it, so it does not go into the loop above and tcmulib_processing_complete is never called for it.

If the race analysis is correct I think we just need to do something like the attached where we hold the device completion lock while updating the aio track calls as well as doing the tcmulib_command_complete. aio_command_finish it just a little funky in that it straddles both the aio code and tcmur cmd handler code, so my change here is a little gross.

Jasons392-plus-my-extra-dumb-locking.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couldn't the call to tcmur_command_complete and track_aio_request_finish be swapped?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't the call to tcmur_command_complete and track_aio_request_finish be swapped?

Lol, yeah that is much better! My eyes were probably going crosseyed :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK -- swapped the two function calls.


static int alloc_iovec(struct tcmulib_cmd *cmd, size_t length)
Expand Down