Hi,

Overall I the patch looks good to me. I have a few comments below.

On 20/02/2016 13:00, Parav Pandit wrote:

Resource pool is created/destroyed dynamically whenever
charging/uncharging occurs respectively and whenever user
configuration is done. Its a tradeoff of memory vs little more code

Its -> It's

space that creates resource pool whenever necessary,
instead of creating them during cgroup creation and device registration
time.

Signed-off-by: Parav Pandit <redacted>

+struct rdmacg_device {
+	struct rdmacg_pool_info pool_info;
+	struct list_head	rdmacg_list;
+	struct list_head	rpool_head;
+	spinlock_t		rpool_lock;	/* protects rsource pool list */

rsource -> resource

+	char			*name;
+};
+
+/* APIs for RDMA/IB stack to publish when a device wants to
+ * participate in resource accounting
+ */
+int rdmacg_register_device(struct rdmacg_device *device);
+void rdmacg_unregister_device(struct rdmacg_device *device);
+
+/* APIs for RDMA/IB stack to charge/uncharge pool specific resources */
+int rdmacg_try_charge(struct rdma_cgroup **rdmacg,
+		      struct rdmacg_device *device,
+		      int resource_index,
+		      int num);
+void rdmacg_uncharge(struct rdma_cgroup *cg,
+		     struct rdmacg_device *device,
+		     int resource_index,
+		     int num);
+void rdmacg_query_limit(struct rdmacg_device *device,
+			int *limits, int max_count);

You can drop the max_count parameter, and require the caller to
always provide pool_info->table_len items, couldn't you?

unavailibility -> unavailability

+/**
+ * uncharge_cg_resource - uncharge resource for rdma cgroup
+ * @cg: pointer to cg to uncharge and all parents in hierarchy

It only uncharges a single cg, right?

+ * @device: pointer to ib device
+ * @index: index of the resource to uncharge in cg (resource pool)
+ * @num: the number of rdma resource to uncharge
+ *
+ * It also frees the resource pool in the hierarchy for the resource pool
+ * which was created as part of charing operation.

charing -> charging

+ */
+static void uncharge_cg_resource(struct rdma_cgroup *cg,
+				 struct rdmacg_device *device,
+				 int index, int num)
+{
+	struct rdmacg_resource_pool *rpool;
+	struct rdmacg_pool_info *pool_info = &device->pool_info;
+
+	spin_lock(&cg->rpool_list_lock);
+	rpool = find_cg_rpool_locked(cg, device);

Is it possible for rpool to be NULL?

+
+	/*
+	 * A negative count (or overflow) is invalid,
+	 * it indicates a bug in the rdma controller.
+	 */
+	rpool->resources[index].usage -= num;
+
+	WARN_ON_ONCE(rpool->resources[index].usage < 0);
+	rpool->refcnt--;
+	if (rpool->refcnt == 0 && rpool->num_max_cnt == pool_info->table_len) {
+		/*
+		 * No user of the rpool and all entries are set to max, so
+		 * safe to delete this rpool.
+		 */
+		list_del(&rpool->cg_list);
+		spin_unlock(&cg->rpool_list_lock);
+
+		free_cg_rpool(rpool);
+		return;
+	}
+	spin_unlock(&cg->rpool_list_lock);
+}

+/**
+ * charge_cg_resource - charge resource for rdma cgroup
+ * @cg: pointer to cg to charge
+ * @device: pointer to rdmacg device
+ * @index: index of the resource to charge in cg (resource pool)
+ * @num: the number of rdma resource to charge
+ */
+static int charge_cg_resource(struct rdma_cgroup *cg,
+			      struct rdmacg_device *device,
+			      int index, int num)
+{
+	struct rdmacg_resource_pool *rpool;
+	s64 new;
+	int ret = 0;
+
+retry:
+	spin_lock(&cg->rpool_list_lock);
+	rpool = find_cg_rpool_locked(cg, device);
+	if (!rpool) {
+		spin_unlock(&cg->rpool_list_lock);
+		ret = alloc_cg_rpool(cg, device);
+		if (ret)
+			goto err;
+		else
+			goto retry;

Instead of retrying after allocation of a new rpool, why not just return the
newly allocated rpool (or the existing one) from alloc_cg_rpool?

+	}
+	new = num + rpool->resources[index].usage;
+	if (new > rpool->resources[index].max) {
+		ret = -EAGAIN;
+	} else {
+		rpool->refcnt++;
+		rpool->resources[index].usage = new;
+	}
+	spin_unlock(&cg->rpool_list_lock);
+err:
+	return ret;
+}

+static ssize_t rdmacg_resource_set_max(struct kernfs_open_file *of,
+				       char *buf, size_t nbytes, loff_t off)
+{
+	struct rdma_cgroup *cg = css_rdmacg(of_css(of));
+	const char *dev_name;
+	struct rdmacg_resource_pool *rpool;
+	struct rdmacg_device *device;
+	char *options = strstrip(buf);
+	struct rdmacg_pool_info *pool_info;
+	u64 enables = 0;

This limits the number of resources to 64. Sounds fine to me, but I think 
there should be a check somewhere (maybe in rdmacg_register_device()?) to
make sure someone doesn't pass too many resources.
 

+	int *new_limits;
+	int i = 0, ret = 0;
+
+	/* extract the device name first */
+	dev_name = strsep(&options, " ");
+	if (!dev_name) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* acquire lock to synchronize with hot plug devices */
+	mutex_lock(&dev_mutex);
+
+	device = rdmacg_get_device_locked(dev_name);
+	if (!device) {
+		ret = -ENODEV;
+		goto parse_err;
+	}
+
+	pool_info = &device->pool_info;
+
+	new_limits = kcalloc(pool_info->table_len, sizeof(int), GFP_KERNEL);
+	if (!new_limits) {
+		ret = -ENOMEM;
+		goto parse_err;
+	}
+
+	ret = rdmacg_parse_limits(options, pool_info, new_limits, &enables);
+	if (ret)
+		goto opt_err;
+
+retry:
+	spin_lock(&cg->rpool_list_lock);
+	rpool = find_cg_rpool_locked(cg, device);
+	if (!rpool) {
+		spin_unlock(&cg->rpool_list_lock);
+		ret = alloc_cg_rpool(cg, device);
+		if (ret)
+			goto opt_err;
+		else
+			goto retry;

You can avoid the retry here too. Perhaps this can go into a function.

+	}
+
+	/* now set the new limits of the rpool */
+	while (enables) {
+		/* if user set the limit, enables bit is set */
+		if (enables & BIT(i)) {
+			enables &= ~BIT(i);
+			set_resource_limit(rpool, i, new_limits[i]);
+		}
+		i++;
+	}
+	if (rpool->refcnt == 0 &&
+	    rpool->num_max_cnt == pool_info->table_len) {
+		/*
+		 * No user of the rpool and all entries are
+		 * set to max, so safe to delete this rpool.
+		 */
+		list_del(&rpool->cg_list);
+		spin_unlock(&cg->rpool_list_lock);
+		free_cg_rpool(rpool);
+	} else {
+		spin_unlock(&cg->rpool_list_lock);
+	}

You should consider putting this piece of code in a function (the
check of the reference counts and release of the rpool).

+
+opt_err:
+	kfree(new_limits);
+parse_err:
+	mutex_unlock(&dev_mutex);
+err:
+	return ret ?: nbytes;
+}
+

+
+static int print_rpool_values(struct seq_file *sf,

This can return void.

+			      struct rdmacg_pool_info *pool_info,
+			      u32 *value_tbl)
+{
+	int i;
+
+	for (i = 0; i < pool_info->table_len; i++) {
+		seq_puts(sf, pool_info->resource_name_table[i]);
+		seq_putc(sf, '=');
+		if (value_tbl[i] == S32_MAX)
+			seq_puts(sf, RDMACG_MAX_STR);
+		else
+			seq_printf(sf, "%d", value_tbl[i]);
+		seq_putc(sf, ' ');
+	}
+	return 0;
+}
+

Thanks,
Haggai