diff mbox series

[EXPERIMENTAL,v1,2/4] RDMA/loopback: Add helper lib for resources and cqe fifo

Message ID 1551248837-64041-3-git-send-email-parav@mellanox.com (mailing list archive)
State RFC
Headers show
Series RDMA loopback device | expand

Commit Message

Parav Pandit Feb. 27, 2019, 6:27 a.m. UTC
Implement xarray based resource table ids for CQ, QP and MR.

Implement Fifo for CQEs and QP's RQEs to be reused later in SRQ.

Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 drivers/infiniband/sw/loopback/helper.c          | 139 +++++++++++++++++++++++
 drivers/infiniband/sw/loopback/loopback_helper.h |  68 +++++++++++
 2 files changed, 207 insertions(+)
 create mode 100644 drivers/infiniband/sw/loopback/helper.c
 create mode 100644 drivers/infiniband/sw/loopback/loopback_helper.h

Comments

Bart Van Assche March 1, 2019, 6:16 p.m. UTC | #1
On Wed, 2019-02-27 at 00:27 -0600, Parav Pandit wrote:
> +#include <linux/completion.h>
> +struct loopback_resource {
> +	struct completion completion;
> +	/* wait for all datapath references to drop, we don't want to do
> +	 * large memcpy while holding rcu, refcount doesn't hurt the
> +	 * performance.
> +	 */
> +	refcount_t refcount;

Please consider struct kref instead of open-coding the kref implementation.

> +/* TODO: replace with kernel pfifo, which consumes more memory */
> +struct loopback_fifo {
> +	/* Protect insert, remove to list */
> +	spinlock_t lock;
> +	/* head of entries, FIFO order */
> +	struct list_head list;
> +	u32 entries;
> +};

How do pfifo and kfifo compare and which of the two is the most appropriate
for this driver?

Thanks,

Bart.
Parav Pandit March 1, 2019, 7:47 p.m. UTC | #2
Hi Bart,

> -----Original Message-----
> From: Bart Van Assche
> Sent: Friday, March 1, 2019 12:17 PM
> To: Parav Pandit ; linux-rdma@vger.kernel.org
> Subject: Re: [EXPERIMENTAL v1 2/4] RDMA/loopback: Add helper lib for
> resources and cqe fifo
> 
> On Wed, 2019-02-27 at 00:27 -0600, Parav Pandit wrote:
> > +#include <linux/completion.h>
> > +struct loopback_resource {
> > +	struct completion completion;
> > +	/* wait for all datapath references to drop, we don't want to do
> > +	 * large memcpy while holding rcu, refcount doesn't hurt the
> > +	 * performance.
> > +	 */
> > +	refcount_t refcount;
> 
> Please consider struct kref instead of open-coding the kref implementation.
> 
In the current approach QP's refcount drops to zero, when in modify_qp state so if incoming request arrives for this QP under state change, get_table_entry_for_id() returns an error in datapath, this error results into retry_exceeded_error.
We don't want to free the QP struct at this point in modify_qp().

Do not wanted to hold the QP's spinlock either while doing memcpy().
Also there are two QPs involved whose state can change, and both QPs can be working on send/recv.
So spin locking QP on each side will results into A-B, B-A locking sequence into deadlock.
So current design does with refcount of QP and modify_qp waits until post_sends() are done.

So refcount is not really keeping the life of an object (for which kref are better choice) from memory perspective.

Any alternative lock less ideas would you suggest?

> > +/* TODO: replace with kernel pfifo, which consumes more memory */
> > +struct loopback_fifo {
> > +	/* Protect insert, remove to list */
> > +	spinlock_t lock;
> > +	/* head of entries, FIFO order */
> > +	struct list_head list;
> > +	u32 entries;
> > +};
> 
> How do pfifo and kfifo compare and which of the two is the most
> appropriate for this driver?
> 
I didn't recall at time of writing the TODO, what was the actual API, but knew something I used in past that is available for this exact need. include/linux/kfifo.h should be used with write lock due to sender and reciver and srq sharing the cq.
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/loopback/helper.c b/drivers/infiniband/sw/loopback/helper.c
new file mode 100644
index 0000000..1294338
--- /dev/null
+++ b/drivers/infiniband/sw/loopback/helper.c
@@ -0,0 +1,139 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/*
+ * Copyright (c) 2019 Mellanox Technologies. All rights reserved.
+ */
+
+#include <linux/xarray.h>
+#include <linux/refcount.h>
+#include "loopback_helper.h"
+
+void init_table(struct loopback_resource_table *table,
+		u32 min_id, u32 max_id, u32 id_mask, u8 id_shift)
+{
+	spin_lock_init(&table->lock);
+	xa_init_flags(&table->ids, XA_FLAGS_ALLOC);
+	table->max_id = max_id;
+	table->min_id = min_id;
+	table->id_mask = id_mask;
+	table->id_shift = id_shift;
+}
+
+void put_table_entry(struct loopback_resource *entry)
+{
+	if (refcount_dec_and_test(&entry->refcount))
+		complete(&entry->completion);
+}
+
+struct loopback_resource *
+get_table_entry_by_id(struct loopback_resource_table *table, u32 id)
+{
+	struct loopback_resource *entry;
+
+	id = (id & table->id_mask) >> table->id_shift;
+	rcu_read_lock();
+	entry = xa_load(&table->ids, id);
+	if (entry) {
+		if (!xa_get_mark(&table->ids, id,
+				 LOOPBACK_RESOURCE_STATE_VALID) ||
+		    !refcount_inc_not_zero(&entry->refcount))
+			entry = NULL;
+	}
+	rcu_read_unlock();
+	return entry;
+}
+
+int _attach_table_id(struct loopback_resource_table *table,
+		     struct loopback_resource *entry,
+		     int desired_id, bool valid)
+{
+	u32 start_id = table->min_id;
+	u32 stop_id = table->max_id;
+	int ret;
+
+	refcount_set(&entry->refcount, 1);
+	init_completion(&entry->completion);
+
+	if (desired_id > 0) {
+		/* need the exact match id */
+		start_id = desired_id << table->id_shift;
+		stop_id = desired_id << table->id_shift;
+	}
+	entry->id = start_id;
+	ret = xa_alloc(&table->ids, &entry->id, stop_id, entry, GFP_KERNEL);
+	if (ret)
+		return ret;
+	if (valid)
+		xa_set_mark(&table->ids, entry->id,
+			    LOOPBACK_RESOURCE_STATE_VALID);
+	return 0;
+}
+
+int attach_table_id_for_id(struct loopback_resource_table *table,
+			   struct loopback_resource *entry, int desired_id)
+{
+	return _attach_table_id(table, entry, desired_id, true);
+}
+
+int attach_table_id_free_state(struct loopback_resource_table *table,
+			       struct loopback_resource *entry)
+{
+	return _attach_table_id(table, entry, -1, false);
+}
+
+int attach_table_id(struct loopback_resource_table *table,
+		    struct loopback_resource *entry)
+{
+	return _attach_table_id(table, entry, -1, true);
+}
+
+void detach_table_id(struct loopback_resource_table *table,
+		     struct loopback_resource *entry)
+{
+	/* Mark entry invalid, so no new references can be taken */
+	xa_clear_mark(&table->ids, entry->id,
+		      LOOPBACK_RESOURCE_STATE_VALID);
+
+	/* Now wait for all readers to stop using the resource */
+	synchronize_rcu();
+
+	put_table_entry(entry);
+	wait_for_completion(&entry->completion);
+	/* At this point there cannot be any active reader */
+	WARN_ON(refcount_read(&entry->refcount));
+
+	/* Now that all references dropped, its free to reassign this id */
+	xa_erase(&table->ids, entry->id);
+}
+
+void init_fifo(struct loopback_fifo *fifo)
+{
+	spin_lock_init(&fifo->lock);
+	INIT_LIST_HEAD(&fifo->list);
+}
+
+void push_to_fifo(struct loopback_fifo *fifo, struct list_head *entry)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&fifo->lock, flags);
+	list_add_tail(entry, &fifo->list);
+	fifo->entries++;
+	spin_unlock_irqrestore(&fifo->lock, flags);
+}
+
+struct list_head *pop_from_fifo(struct loopback_fifo *fifo)
+{
+	struct list_head *entry = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fifo->lock, flags);
+	if (!list_empty(&fifo->list)) {
+		entry = fifo->list.next;
+		list_del(fifo->list.next);
+		fifo->entries--;
+	}
+	if (!entry && fifo->entries)
+		pr_debug("%s fifo entries = %u\n", __func__,  fifo->entries);
+	spin_unlock_irqrestore(&fifo->lock, flags);
+	return entry;
+}
diff --git a/drivers/infiniband/sw/loopback/loopback_helper.h b/drivers/infiniband/sw/loopback/loopback_helper.h
new file mode 100644
index 0000000..8ea93f4
--- /dev/null
+++ b/drivers/infiniband/sw/loopback/loopback_helper.h
@@ -0,0 +1,68 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/*
+ * Copyright (c) 2019 Mellanox Technologies. All rights reserved.
+ */
+
+#ifndef LOOPBACK_HELPER_H
+#define LOOPBACK_HELPER_H
+
+#include <linux/completion.h>
+struct loopback_resource {
+	struct completion completion;
+	/* wait for all datapath references to drop, we don't want to do
+	 * large memcpy while holding rcu, refcount doesn't hurt the
+	 * performance.
+	 */
+	refcount_t refcount;
+	u32 id;
+};
+
+#define LOOPBACK_RESOURCE_STATE_VALID XA_MARK_1
+
+/**
+ * struct loopback_resource_table - resource table
+ */
+
+struct loopback_resource_table {
+	/* Protects xarray of resources (ids) */
+	spinlock_t lock;
+	struct xarray ids;
+	u32 min_id;
+	u32 max_id;
+	u32 id_mask;
+	u8 id_shift;
+};
+
+/* TODO: replace with kernel pfifo, which consumes more memory */
+struct loopback_fifo {
+	/* Protect insert, remove to list */
+	spinlock_t lock;
+	/* head of entries, FIFO order */
+	struct list_head list;
+	u32 entries;
+};
+
+static inline u64 get_fifo_entries(struct loopback_fifo *fifo)
+{
+	return fifo->entries;
+}
+
+void init_fifo(struct loopback_fifo *fifo);
+void push_to_fifo(struct loopback_fifo *fifo, struct list_head *entry);
+struct list_head *pop_from_fifo(struct loopback_fifo *fifo);
+
+void init_table(struct loopback_resource_table *table,
+		u32 min_id, u32 max_id, u32 id_mask, u8 id_shift);
+struct loopback_resource *
+get_table_entry_by_id(struct loopback_resource_table *table, u32 id);
+void put_table_entry(struct loopback_resource *entry);
+int attach_table_id_for_id(struct loopback_resource_table *table,
+			   struct loopback_resource *entry, int desired_id);
+int attach_table_id_free_state(struct loopback_resource_table *table,
+			       struct loopback_resource *entry);
+int attach_table_id(struct loopback_resource_table *table,
+		    struct loopback_resource *entry);
+void detach_table_id(struct loopback_resource_table *table,
+		     struct loopback_resource *entry);
+
+#endif