Re: [igt-dev] [PATCH i-g-t 12/14] benchmarks/gem_wsim: extract prepare contexts code to new function

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 26/09/2023 09:44, Marcin Bernatowicz wrote:
> No functional changes.
> Extracted prepare_contexts function from prepare_workload.
> Small code cleanup for "No need for 'else' after continue/break". (Kamil)
> 
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> ---
>   benchmarks/gem_wsim.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index 2c6ccd3a9..55f8d9b1b 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -1766,20 +1766,13 @@ static void measure_active_set(struct workload *wrk)
>   
>   #define alloca0(sz) ({ size_t sz__ = (sz); memset(alloca(sz__), 0, sz__); })
>   
> -static int prepare_workload(unsigned int id, struct workload *wrk)
> +static int prepare_contexts(unsigned int id, struct workload *wrk)
>   {
> -	struct working_set **sets;
> -	unsigned long total = 0;
>   	uint32_t share_vm = 0;
>   	int max_ctx = -1;
>   	struct w_step *w;
>   	int i, j;
>   
> -	wrk->id = id;
> -	wrk->bb_prng = (wrk->flags & SYNCEDCLIENTS) ? master_prng : rand();
> -	wrk->bo_prng = (wrk->flags & SYNCEDCLIENTS) ? master_prng : rand();
> -	wrk->run = true;
> -
>   	/*
>   	 * Pre-scan workload steps to allocate context list storage.
>   	 */
> @@ -1968,6 +1961,23 @@ static int prepare_workload(unsigned int id, struct workload *wrk)
>   	if (share_vm)
>   		vm_destroy(fd, share_vm);
>   
> +	return 0;
> +}
> +
> +static int prepare_workload(unsigned int id, struct workload *wrk)
> +{
> +	struct working_set **sets;
> +	unsigned long total = 0;
> +	struct w_step *w;
> +	int i, j;
> +
> +	wrk->id = id;
> +	wrk->bb_prng = (wrk->flags & SYNCEDCLIENTS) ? master_prng : rand();
> +	wrk->bo_prng = (wrk->flags & SYNCEDCLIENTS) ? master_prng : rand();
> +	wrk->run = true;
> +
> +	prepare_contexts(id, wrk);
> +
>   	/* Record default preemption. */
>   	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
>   		if (w->type == BATCH)
> @@ -1990,9 +2000,9 @@ static int prepare_workload(unsigned int id, struct workload *wrk)
>   
>   			if (w2->context != w->context)
>   				continue;
> -			else if (w2->type == PREEMPTION)
> +			if (w2->type == PREEMPTION)
>   				break;
> -			else if (w2->type != BATCH)
> +			if (w2->type != BATCH)

To me it is more readable like it was. Is this some general rule that 
else if should not be used in such cases?

Either case I'd be happiest if extracting code into functions wash't 
mixed with unrelated changes.

Otherwise code extraction looks fine.

Regards,

Tvrtko

>   				continue;
>   
>   			w2->preempt_us = w->period;