On 6/20/19 8:03 AM, Jack Wang wrote:
+#define P1 )
+#define P2 ))
+#define P3 )))
+#define P4 ))))
+#define P(N) P ## N
+
+#define CAT(a, ...) PRIMITIVE_CAT(a, __VA_ARGS__)
+#define PRIMITIVE_CAT(a, ...) a ## __VA_ARGS__
+
+#define LIST(...) \
+ __VA_ARGS__, \
+ ({ unknown_type(); NULL; }) \
+ CAT(P, COUNT_ARGS(__VA_ARGS__)) \
+
+#define EMPTY()
+#define DEFER(id) id EMPTY()
+
+#define _CASE(obj, type, member) \
+ __builtin_choose_expr( \
+ __builtin_types_compatible_p( \
+ typeof(obj), type), \
+ ((type)obj)->member
+#define CASE(o, t, m) DEFER(_CASE)(o, t, m)
+
+/*
+ * Below we define retrieving of sessname from common IBTRS types.
+ * Client or server related types have to be defined by special
+ * TYPES_TO_SESSNAME macro.
+ */
+
+void unknown_type(void);
+
+#ifndef TYPES_TO_SESSNAME
+#define TYPES_TO_SESSNAME(...) ({ unknown_type(); NULL; })
+#endif
+
+#define ibtrs_prefix(obj) \
+ _CASE(obj, struct ibtrs_con *, sess->sessname), \
+ _CASE(obj, struct ibtrs_sess *, sessname), \
+ TYPES_TO_SESSNAME(obj) \
+ ))
No preprocessor voodoo please. Please remove all of the above and modify
the logging statements such that these pass the proper name string as
first argument to logging macros.
+struct ibtrs_msg_conn_req {
+ u8 __cma_version; /* Is set to 0 by cma.c in case of
+ * AF_IB, do not touch that. */
+ u8 __ip_version; /* On sender side that should be
+ * set to 0, or cma_save_ip_info()
+ * extract garbage and will fail. */
+ __le16 magic;
+ __le16 version;
+ __le16 cid;
+ __le16 cid_num;
+ __le16 recon_cnt;
+ uuid_t sess_uuid;
+ uuid_t paths_uuid;
+ u8 reserved[12];
+};
Please remove the reserved[] array and check private_data_len in the
code that receives the login request.
+/**
+ * struct ibtrs_msg_conn_rsp - Server connection response to the client
+ * @magic: IBTRS magic
+ * @version: IBTRS protocol version
+ * @errno: If rdma_accept() then 0, if rdma_reject() indicates error
+ * @queue_depth: max inflight messages (queue-depth) in this session
+ * @max_io_size: max io size server supports
+ * @max_hdr_size: max msg header size server supports
+ *
+ * NOTE: size is 56 bytes, max possible is 136 bytes, see man rdma_accept().
+ */
+struct ibtrs_msg_conn_rsp {
+ __le16 magic;
+ __le16 version;
+ __le16 errno;
+ __le16 queue_depth;
+ __le32 max_io_size;
+ __le32 max_hdr_size;
+ u8 reserved[40];
+};
Same comment here: please remove the reserved[] array and check
private_data_len in the code that processes this data structure.
+static inline int sockaddr_cmp(const struct sockaddr *a,
+ const struct sockaddr *b)
+{
+ switch (a->sa_family) {
+ case AF_IB:
+ return memcmp(&((struct sockaddr_ib *)a)->sib_addr,
+ &((struct sockaddr_ib *)b)->sib_addr,
+ sizeof(struct ib_addr));
+ case AF_INET:
+ return memcmp(&((struct sockaddr_in *)a)->sin_addr,
+ &((struct sockaddr_in *)b)->sin_addr,
+ sizeof(struct in_addr));
+ case AF_INET6:
+ return memcmp(&((struct sockaddr_in6 *)a)->sin6_addr,
+ &((struct sockaddr_in6 *)b)->sin6_addr,
+ sizeof(struct in6_addr));
+ default:
+ return -ENOENT;
+ }
+}
+
+static inline int sockaddr_to_str(const struct sockaddr *addr,
+ char *buf, size_t len)
+{
+ int cnt;
+
+ switch (addr->sa_family) {
+ case AF_IB:
+ cnt = scnprintf(buf, len, "gid:%pI6",
+ &((struct sockaddr_ib *)addr)->sib_addr.sib_raw);
+ return cnt;
+ case AF_INET:
+ cnt = scnprintf(buf, len, "ip:%pI4",
+ &((struct sockaddr_in *)addr)->sin_addr);
+ return cnt;
+ case AF_INET6:
+ cnt = scnprintf(buf, len, "ip:%pI6c",
+ &((struct sockaddr_in6 *)addr)->sin6_addr);
+ return cnt;
+ }
+ cnt = scnprintf(buf, len, "<invalid address family>");
+ pr_err("Invalid address family\n");
+ return cnt;
+}
Since these functions are not in the hot path, please move these into a
.c file.
+/**
+ * ibtrs_invalidate_flag() - returns proper flags for invalidation
+ *
+ * NOTE: This function is needed for compat layer, so think twice before
+ * rename or remove.
+ */
+static inline u32 ibtrs_invalidate_flag(void)
+{
+ return IBTRS_MSG_NEED_INVAL_F;
+}
An inline function that does nothing else than returning a compile-time
constant? That does not look useful to me. How about inlining this function?
+#define STAT_STORE_FUNC(type, store, reset) \
+static ssize_t store##_store(struct kobject *kobj, \
+ struct kobj_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ int ret = -EINVAL; \
+ type *sess = container_of(kobj, type, kobj_stats); \
+ \
+ if (sysfs_streq(buf, "1")) \
+ ret = reset(&sess->stats, true); \
+ else if (sysfs_streq(buf, "0")) \
+ ret = reset(&sess->stats, false); \
+ if (ret) \
+ return ret; \
+ \
+ return count; \
+}
The above macro concatenates the suffix "_store" to a macro argument
with the name 'store'. Please chose a less confusing name for the macro
argument. Additionally, using 'reset' for the name of an macro argument
that is a function that stores a value seems confusing to me. How about
renaming that macro argument into 'set' or 'store_value'?
+#define STAT_SHOW_FUNC(type, show, print) \
+static ssize_t show##_show(struct kobject *kobj, \
+ struct kobj_attribute *attr, \
+ char *page) \
+{ \
+ type *sess = container_of(kobj, type, kobj_stats); \
+ \
+ return print(&sess->stats, page, PAGE_SIZE); \
+}
Same comment for the macro argument 'show' in the above function.
Thanks,
Bart.
