Hi Sagi,

On Mon, Feb 5, 2018 at 11:59 AM, Sagi Grimberg [off-list ref] wrote:

Hi Roman,

quoted

+struct ibtrs_clt_io_req {
+       struct list_head        list;
+       struct ibtrs_iu         *iu;
+       struct scatterlist      *sglist; /* list holding user data */
+       unsigned int            sg_cnt;
+       unsigned int            sg_size;
+       unsigned int            data_len;
+       unsigned int            usr_len;
+       void                    *priv;
+       bool                    in_use;
+       struct ibtrs_clt_con    *con;
+       union {
+               struct ib_pool_fmr      **fmr_list;
+               struct ibtrs_fr_desc    **fr_list;
+       };

We are pretty much stuck with fmrs for legacy devices, it has
no future support plans, please don't add new dependencies
on it. Its already hard enough to get rid of it.

Got it, we have a plan to get rid of fmr.  But as I remember our
internal tests: fr is slower.  The question: why that can be
according to your experience?  I will retest, but still that is
interesting to know.

quoted

+       void                    *map_page;
+       struct ibtrs_tag        *tag;

Can I ask why do you need another tag that is not the request
tag?

Once I responded already, the summary is the following:

1. Indeed mq supports tags sharing, but only between hw queues, not
globally, so for us that means tags->nr_hw_queues = 1, which kills
performance.

2. We need tags sharing in the transport library, which should not
be tightly coupled with block device.

quoted

+       u16                     nmdesc;
+       enum dma_data_direction dir;
+       ibtrs_conf_fn           *conf;
+       unsigned long           start_time;
+};
+

quoted

+static inline struct ibtrs_clt_con *to_clt_con(struct ibtrs_con *c)
+{
+       if (unlikely(!c))
+               return NULL;
+
+       return container_of(c, struct ibtrs_clt_con, c);
+}
+
+static inline struct ibtrs_clt_sess *to_clt_sess(struct ibtrs_sess *s)
+{
+       if (unlikely(!s))
+               return NULL;
+
+       return container_of(s, struct ibtrs_clt_sess, s);
+}

Seems a bit awkward that container_of wrappers check pointer validity...

That can be fixed, frankly, I don't remember code paths where I
implicitly rely on that returned null: session or connection are
always expected as valid pointers.

quoted

+/**
+ * list_next_or_null_rr - get next list element in round-robin fashion.
+ * @pos:     entry, starting cursor.
+ * @head:    head of the list to examine. This list must have at least
one
+ *           element, namely @pos.
+ * @member:  name of the list_head structure within typeof(*pos).
+ *
+ * Important to understand that @pos is a list entry, which can be
already
+ * removed using list_del_rcu(), so if @head has become empty NULL will
be
+ * returned. Otherwise next element is returned in round-robin fashion.
+ */
+#define list_next_or_null_rcu_rr(pos, head, member) ({                 \
+       typeof(pos) ________next = NULL;                                \
+                                                                       \
+       if (!list_empty(head))                                          \
+               ________next = (pos)->member.next != (head) ?           \
+                       list_entry_rcu((pos)->member.next,              \
+                                      typeof(*pos), member) :          \
+                       list_entry_rcu((pos)->member.next->next,        \
+                                      typeof(*pos), member);           \
+       ________next;                                                   \
+})

Why is this local to your driver?

Yeah, of course I can try to extend list.h

quoted

+
+/* See ibtrs-log.h */
+#define TYPES_TO_SESSNAME(obj)                                         \
+       LIST(CASE(obj, struct ibtrs_clt_sess *, s.sessname),            \
+            CASE(obj, struct ibtrs_clt *, sessname))
+
+#define TAG_SIZE(clt) (sizeof(struct ibtrs_tag) + (clt)->pdu_sz)
+#define GET_TAG(clt, idx) ((clt)->tags + TAG_SIZE(clt) * idx)

Still don't understand why this is even needed..

--
Roman