On Mon, Apr 27, 2020 at 04:18:13PM +0200, Emanuele Giuseppe Esposito wrote:
> +static struct statsfs_value *find_value(struct statsfs_value_source *src,
> + struct statsfs_value *val)
> +{
> + struct statsfs_value *entry;
> +
> + for (entry = src->values; entry->name; entry++) {
> + if (entry == val) {
> + WARN_ON(strcmp(entry->name, val->name) != 0);
Umm. 'entry' and 'val' are pointers. So if entry is equal to val,
how could entry->name and val->name not be the same thing?
> +/* Called with rwsem held for writing */
> +static struct statsfs_value_source *create_value_source(void *base)
> +{
> + struct statsfs_value_source *val_src;
> +
> + val_src = kzalloc(sizeof(struct statsfs_value_source), GFP_KERNEL);
> + if (!val_src)
> + return ERR_PTR(-ENOMEM);
> +
> + val_src->base_addr = base;
> + val_src->list_element =
> + (struct list_head)LIST_HEAD_INIT(val_src->list_element);
This is not how LIST_HEAD_INIT is generally used, but see below.
> +int statsfs_source_add_values(struct statsfs_source *source,
> + struct statsfs_value *stat, void *ptr)
> +{
> + struct statsfs_value_source *val_src;
> + struct statsfs_value_source *entry;
> +
> + down_write(&source->rwsem);
> +
> + list_for_each_entry(entry, &source->values_head, list_element) {
> + if (entry->base_addr == ptr && entry->values == stat) {
> + up_write(&source->rwsem);
> + return -EEXIST;
> + }
> + }
> +
> + val_src = create_value_source(ptr);
> + val_src->values = (struct statsfs_value *)stat;
> +
> + /* add the val_src to the source list */
> + list_add(&val_src->list_element, &source->values_head);
> +
> + up_write(&source->rwsem);
I dislike this use of doubly linked lists. I would suggest using an
allocating XArray to store your values. Something like this:
+int statsfs_source_add_values(struct statsfs_source *source,
+ struct statsfs_value *stat, void *ptr)
+{
+ struct statsfs_value_source *entry, *val_src;
+ unsigned long index;
+ int err = -EEXIST;
+
+ val_src = create_value_source(ptr);
+ val_src->values = stat;
+
+ xa_lock(&source->values);
+ xa_for_each(&source->values, index, entry) {
+ if (entry->base_addr == ptr && entry->values == stat)
+ goto out;
+ }
+
+ err = __xa_alloc(&source->values, &val_src->id, val_src, xa_limit_32b,
+ GFP_KERNEL);
+out:
+ xa_unlock(&source->values);
+ if (err)
+ kfree(val_src);
+ return err;
+}
Using an XArray avoids the occasional latency problems you can see with
rwsems, as well as being more cache-effective than a linked list.
