[Devel] [PATCH rh7] ve: Kill ve_list_head and ve_struct::ve_list

[Vladimir Davydov]

On Thu, Sep 24, 2015 at 06:11:26PM +0300, Kirill Tkhai wrote:
> Since we use ve_idr layer to reserve a id for a ve,
> and since a ve is linked there, using of ve_list_head
> just for linking VEs becomes redundant.

Nevertheless, iterating over a list is more convenient than over idr
IMO.

> 
> This patch replaces ve_list_head in the places, we iterate
> thru VEs list, with ve_idr mechanish, and kills the
> duplicate manner.

AFAICS this patch doesn't improve performance neither does it make the
code more readable IMHO, so personally I would refrain from merging it.
Up to Konstantin.

Also, see a few comments regarding the implementation below.

> 
> Signed-off-by: Kirill Tkhai <ktkhai at odin.com>
> ---
...
> @@ -49,10 +49,9 @@ void vzmon_unregister_veaddr_print_cb(ve_seq_print_t);
>  int venet_init(void);
>  #endif
>  
> -extern struct list_head ve_list_head;
> -#define for_each_ve(ve)	list_for_each_entry((ve), &ve_list_head, ve_list)

I wouldn't drop the macro.

>  extern struct mutex ve_list_lock;

There's no ve_list, but there's still ve_list_lock. Confusing. Same for
ve_list_add and ve_list_del.

>  extern struct ve_struct *get_ve_by_id(envid_t);
> +extern struct idr ve_idr;
>  extern struct cgroup *ve_cgroup_open(struct cgroup *root, int flags, envid_t veid);
>  extern int ve_cgroup_remove(struct cgroup *root, envid_t veid);
>  
...
> @@ -772,26 +768,24 @@ static int vestat_seq_show(struct seq_file *m, void *v)
>  
>  void *ve_seq_start(struct seq_file *m, loff_t *pos)
>  {
> -	struct ve_struct *curve;
> -
> -	curve = get_exec_env();
>  	mutex_lock(&ve_list_lock);
> -	if (!ve_is_super(curve)) {
> -		if (*pos != 0)
> -			return NULL;
> -		return &curve->ve_list;
> -	}
>  
> -	return seq_list_start(&ve_list_head, *pos);
> +	return ve_seq_next(m, NULL, pos);

I don't think it's correct to increment *pos in seq_start. Look at
seq_read: if the buffer is too small to hold the first entry, we will
jump over it instead of continuing reading it next time seq_read is
called.

>  }
>  EXPORT_SYMBOL(ve_seq_start);
>  
>  void *ve_seq_next(struct seq_file *m, void *v, loff_t *pos)
>  {
> -	if (!ve_is_super(get_exec_env()))
> -		return NULL;
> -	else
> -		return seq_list_next(v, &ve_list_head, pos);
> +	struct ve_struct *ve = get_exec_env();
> +	int id = *pos;
> +
> +	if (!ve_is_super(ve))

AFAICS you forgot to increment *pos here, which might result in
multiplying output inside ve.

> +		return *pos ? NULL : ve;
> +
> +	ve = idr_get_next(&ve_idr, &id);
> +	*pos = id + 1;
> +
> +	return ve;
>  }
>  EXPORT_SYMBOL(ve_seq_next);
>