vdev: log waiting of the first vhost-user request #15
Conversation
vdev.c
Outdated
| vdev->negotiated_protocol_features = 0; | ||
| /* supported_features is being set on handling first GET_FEATURES request */ | ||
| vdev->supported_features = 0; |
There was a problem hiding this comment.
А зачем мы добавили зануление этой переменной?
vdev.c
Outdated
| @@ -591,9 +594,15 @@ static void arm_msg_handling_timer(struct vhd_vdev *vdev, int secs) | |||
| #define MSG_ELAPSED_NSEC_LOG_THRESHOLD (500 * NSEC_PER_MSEC) | |||
| #endif | |||
|
|
|||
|
|
|||
vdev.c
Outdated
| @@ -578,7 +578,10 @@ static void arm_msg_handling_timer(struct vhd_vdev *vdev, int secs) | |||
| timerfd_settime(vdev->timerfd, 0, &itimer, NULL); | |||
| } | |||
|
|
|||
| /* Every so many seconds report if the message is still being handled */ | |||
| /* Report every so many seconds if we haven't received GET_FEATURES request */ | |||
| #define MSG_GET_FEATURES_WAITING_LOG_INTERVAL 30 | |||
There was a problem hiding this comment.
Я бы не добавлял новый енам для этого, особо не вижу смысла, тем более что тут идентичное значение
vdev.c
Outdated
| vhost_req_name(vdev->req), vdev->req, | ||
| (intmax_t)elapsed.tv_sec, elapsed.tv_nsec / NSEC_PER_MSEC); | ||
| } else { | ||
| VHD_OBJ_WARN(vdev, "Still waiting for GET_FEATURES request..."); |
There was a problem hiding this comment.
Я так понимаю на самом деле мы тут ждем первого запроса от VM, а не именно GET_FEATURES. Поэтому я бы писал честно: Still waiting for a vhost-user request..., или типо того
vdev.c
Outdated
| static void vdev_handle_start(struct vhd_vdev *vdev, uint32_t req, | ||
| bool ack_pending) | ||
| { | ||
| /* we need to detach timer_handler only before very first GET_FEATURES request */ | ||
| if (vdev->supported_features == 0) { |
There was a problem hiding this comment.
very first GET_FEATURES request
Тоже не совсем правда, тут вызовется detach на любое количество запросов до GET_FEATURES, выглядит так что может упасть если клиент пришлет любой другой запрос несколько раз. Так не пойдет
|
Да, я ошибочно посчитал, что запрос GET_FEATURES всегда первый, думал смогу обойтись существующими полями в vdev. |
| @@ -594,6 +594,9 @@ static void arm_msg_handling_timer(struct vhd_vdev *vdev, int secs) | |||
| static void vdev_handle_start(struct vhd_vdev *vdev, uint32_t req, | |||
| bool ack_pending) | |||
| { | |||
| /* detach timer_handler attached right after accept, no-op after the first request */ | |||
| vhd_detach_io_handler(vdev->timer_handler); | |||
There was a problem hiding this comment.
Так было сделать проще всего.
Тут остается вот такой редкий кейс: мы задетачили дескриптор из epoll, но таймер все еще возведен и не зафайрился. У нас есть промежуток времени на строках 609-610, когда мы сначала лобавляем timerfd обратно в epoll, а потом переустанавливаем таймер. Теоретически может случиться так, что в этот промежуток времени напишется сообщение Still waiting...
Я думаю, что это почти невозможный кейс и ради него усложнять патч не стоит.
Тем не менее есть такие варианты:
- поменять местами строки 609-610, т.е. сначала делать
arm_msg_handling_timer, а затемvhd_attach_io_handler. Может быть, это и логично. Как раз будет обратный порядок тому, что вvdev_handle_finish. Врядлиepoll_ctlсильно много времени займет в процессе хэндлинга. - сделать в
vhd_detach_io_handlerвозвращаемое значение, которые бы показывало, был ли реально произведен детач или нет, и по нему сделать if тут
There was a problem hiding this comment.
Звучит так что поменять порядок, в общем-то, разумно. Давай наверное так и сделаем тут
zeil
changed the title
Signed-off-by: Dmitrii Iakunin <[email protected]>
No description provided.