Re: [PATCH] sched/eevdf: fix rb augmented with multi fields

[Kayra Cizmeci]

Hello Vincent,

> The eevdf rb tree maintains 3 augmented fields but only one is currently
> copied when balancing the tree.

> Add a more generic define that can be used when there are several augmented
> fields. In this case, we provide a function that takes care of copying all
> fields.

So the problem is we need to copy 3 things and we copy 1 thing currently? (simplified)
Also, I read Prateek's review too, so I'll not go on the same things on this one.

And I thought that 2 separate reviews are better than one, so... :>

> /*
> - * Template for declaring augmented rbtree callbacks (generic case)
> + * Template for declaring augmented rbtree callbacks (generic multi fields)
>   *
>   * RBSTATIC:    'static' or empty
>   * RBNAME:      name of the rb_augment_callbacks structure
>   * RBSTRUCT:    struct type of the tree nodes
>   * RBFIELD:     name of struct rb_node field within RBSTRUCT
> - * RBAUGMENTED: name of field within RBSTRUCT holding data for subtree
> - * RBCOMPUTE:   name of function that recomputes the RBAUGMENTED data
> + * RBCOPY:	name of function that copies the RBAUGMENTED datas
> + * RBCOMPUTE:   name of function that recomputes the RBAUGMENTED datas
>  */

nit. 'datas' is wrong in english. I would use it like that too. Maybe 'fields' but
I think using 'fields' in one of them and 'data' in the others would look bad. 
Ah. English is annoying.

> @@ -114,14 +114,14 @@ RBNAME ## _copy(struct rb_node *rb_old, struct rb_node *rb_new)		\
>  {									\
>  	RBSTRUCT *old = rb_entry(rb_old, RBSTRUCT, RBFIELD);		\
>  	RBSTRUCT *new = rb_entry(rb_new, RBSTRUCT, RBFIELD);		\
> -	new->RBAUGMENTED = old->RBAUGMENTED;				\
> +	RBCOPY(new, old);						\
>  }									\
>  static void								\
>  RBNAME ## _rotate(struct rb_node *rb_old, struct rb_node *rb_new)	\
>  {									\
>  	RBSTRUCT *old = rb_entry(rb_old, RBSTRUCT, RBFIELD);		\
>  	RBSTRUCT *new = rb_entry(rb_new, RBSTRUCT, RBFIELD);		\
> -	new->RBAUGMENTED = old->RBAUGMENTED;				\
> +	RBCOPY(new, old);						\
>  	RBCOMPUTE(old, false);						\
>  }

The bool exit on min_vruntime_update() parameter appears to be unused when I grep or search for
any use case. Could we remove it?

The other things aside these is good for me. It looks really weird to copy only
min_vruntime while not copying others at the first place. And I think the
solution is good. 

I was thinking to do a boot test, but I did not thought it would add something
to the patch. So I did not. I can if it's going to add something tho.

Include if you want to :-):

Reviewed-by: Kayra Cizmeci <kayracizmeci@xxxxxxxxx>