[PATCH v2 1/2]: um: time-travel=ext fixes

[PATCH v2 1/2]: um: time-travel=ext fixes

[Johannes Berg]

Hi,

I had been noticing some hangs in my simulations, and finally had
enough time (and a machine that they reproduce on easily - it's a
timing thing) to debug, so two small fixes for this.

v2:
 * remove change-id ...

johannes




_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH v2 1/2] um: time-travel: fix IRQ handling in time_travel_handle_message()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

As the comment here indicates, we need to do the polling in the
idle loop without blocking interrupts, since interrupts can be
vhost-user messages that we must process even while in our idle
loop.

I don't know why I explained one thing and implemented another,
but we have indeed observed random hangs due to this, depending
on the timing of the messages.

Fixes: 88ce64249233 ("um: Implement time-travel=ext")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/um/kernel/time.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c
index 25eaa6a0c658..c07436e89e59 100644
--- a/arch/um/kernel/time.c
+++ b/arch/um/kernel/time.c
@@ -70,13 +70,17 @@ static void time_travel_handle_message(struct um_timetravel_msg *msg,
 	 * read of the message and write of the ACK.
 	 */
 	if (mode != TTMH_READ) {
+		bool disabled = irqs_disabled();
+
+		BUG_ON(mode == TTMH_IDLE && !disabled);
+
+		if (disabled)
+			local_irq_enable();
 		while (os_poll(1, &time_travel_ext_fd) != 0) {
-			if (mode == TTMH_IDLE) {
-				BUG_ON(!irqs_disabled());
-				local_irq_enable();
-				local_irq_disable();
-			}
+			/* nothing */
 		}
+		if (disabled)
+			local_irq_disable();
 	}
 
 	ret = os_read_file(time_travel_ext_fd, msg, sizeof(*msg));
-- 
2.26.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH v2 2/2] um: time-travel: return the sequence number in ACK messages

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

For external time travel, the protocol says to return the
incoming sequence number in the ACK message to aid debugging,
so do that.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/um/kernel/time.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c
index c07436e89e59..3d109ff3309b 100644
--- a/arch/um/kernel/time.c
+++ b/arch/um/kernel/time.c
@@ -106,6 +106,7 @@ static void time_travel_handle_message(struct um_timetravel_msg *msg,
 		break;
 	}
 
+	resp.seq = msg->seq;
 	os_write_file(time_travel_ext_fd, &resp, sizeof(resp));
 }
 
-- 
2.26.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] um: time-travel: return the sequence number in ACK messages

[Anton Ivanov]

On 10/09/2020 10:31, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> For external time travel, the protocol says to return the
> incoming sequence number in the ACK message to aid debugging,
> so do that.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   arch/um/kernel/time.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c
> index c07436e89e59..3d109ff3309b 100644
> --- a/arch/um/kernel/time.c
> +++ b/arch/um/kernel/time.c
> @@ -106,6 +106,7 @@ static void time_travel_handle_message(struct um_timetravel_msg *msg,
>   		break;
>   	}
>   
> +	resp.seq = msg->seq;
>   	os_write_file(time_travel_ext_fd, &resp, sizeof(resp));
>   }
>   
> 
Acked-By: Anton Ivanov <anton.ivanov@cambridgegreys.com>

-- 
Anton R. Ivanov
https://www.kot-begemot.co.uk/

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 1/2] um: time-travel: fix IRQ handling in time_travel_handle_message()

[Anton Ivanov]

On 10/09/2020 10:31, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> As the comment here indicates, we need to do the polling in the
> idle loop without blocking interrupts, since interrupts can be
> vhost-user messages that we must process even while in our idle
> loop.
> 
> I don't know why I explained one thing and implemented another,
> but we have indeed observed random hangs due to this, depending
> on the timing of the messages.
> 
> Fixes: 88ce64249233 ("um: Implement time-travel=ext")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   arch/um/kernel/time.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c
> index 25eaa6a0c658..c07436e89e59 100644
> --- a/arch/um/kernel/time.c
> +++ b/arch/um/kernel/time.c
> @@ -70,13 +70,17 @@ static void time_travel_handle_message(struct um_timetravel_msg *msg,
>   	 * read of the message and write of the ACK.
>   	 */
>   	if (mode != TTMH_READ) {
> +		bool disabled = irqs_disabled();
> +
> +		BUG_ON(mode == TTMH_IDLE && !disabled);
> +
> +		if (disabled)
> +			local_irq_enable();
>   		while (os_poll(1, &time_travel_ext_fd) != 0) {
> -			if (mode == TTMH_IDLE) {
> -				BUG_ON(!irqs_disabled());
> -				local_irq_enable();
> -				local_irq_disable();
> -			}
> +			/* nothing */
>   		}
> +		if (disabled)
> +			local_irq_disable();
>   	}
>   
>   	ret = os_read_file(time_travel_ext_fd, msg, sizeof(*msg));
> 

Acked-By: Anton Ivanov <anton.ivanov@cambridgegreys.com>

-- 
Anton R. Ivanov
https://www.kot-begemot.co.uk/

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um