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 <[email protected]> > --- ... > @@ -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); > _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
