Re: [PATCH RFC 01/11] um: Make UBD requests synchronous in TT ext/infcpu mode

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2023-11-03 at 16:41 +0000, Benjamin Beichler wrote:
> The UBD driver employs multiple threads to enhance block device accesses
> in userspace. These threads communicate via pipes and are triggered by
> interrupts that utilize the SIGIO handler.
> 
> However, in TT mode, both inf-cpu and external modes, this asynchronous,
> multithreaded request processing lead to issues where requests are not
> processed. This occurs because there is no dedicated time travel handler
> for the UBD interrupt.
> 
> Since asynchronous, multithreaded request processing does not provide
> substantial benefits in time travel mode and may even introduce
> additional overhead (as multiple threads are scheduled sequentially to
> execute requests in TT mode with infinite CPU power), this patch
> switches to synchronous request processing directly within the
> submit_request call for the respective TT modes.

This makes perfect sense. We mostly use hostfs, but recently did the
exact same thing there.

> +#ifdef CONFIG_UML_TIME_TRAVEL_SUPPORT
> +	/* do not initialize asynchronous io-thread and corresponding irq
> +	 * in inf-cpu or ext time travel, as we need synchronous io logic
> +	 */
> +
> +	if (time_travel_mode == TT_MODE_INFCPU ||
> +	    time_travel_mode == TT_MODE_EXTERNAL)

Maybe we should finally refactor these checks though ... :)

And you don't need the ifdef, because time_travel_mode is defined to
TT_MODE_OFF without CONFIG_UML_TIME_TRAVEL_SUPPORT, and then this just
gets optimised out.

>  static int ubd_submit_request(struct ubd *dev, struct request *req)
>  {
> +	int i;
>  	int segs = 0;
>  	struct io_thread_req *io_req;
>  	int ret;
> @@ -1334,6 +1354,17 @@ static int ubd_submit_request(struct ubd *dev, struct request *req)
>  	if (segs)
>  		ubd_map_req(dev, io_req, req);
>  
> +#ifdef CONFIG_UML_TIME_TRAVEL_SUPPORT
> +	//do the request sychronous (bypass io_thread and ubd_handler)
> +	if (time_travel_mode == TT_MODE_INFCPU ||
> +	    time_travel_mode == TT_MODE_EXTERNAL) {
> +		for (i = 0; !io_req->error && i < io_req->desc_cnt; i++)
> +			do_io(io_req, &io_req->io_desc[i]);
> +		finalize_request(io_req);
> +		return 0;
> +	}
> +#endif
> 

Same here, and in fact with the ifdef you'd get an unused variable
warning, but anyway you could move that into the block (or even into the
for now that we require C99!)

johannes

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