From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E27A2C352AB for ; Fri, 27 Sep 2019 08:56:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A5D6A207FF for ; Fri, 27 Sep 2019 08:56:32 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=cloud.ionos.com header.i=@cloud.ionos.com header.b="UjKJwYJr" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725911AbfI0I4c (ORCPT ); Fri, 27 Sep 2019 04:56:32 -0400 Received: from mail-wr1-f67.google.com ([209.85.221.67]:42699 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725946AbfI0I4c (ORCPT ); Fri, 27 Sep 2019 04:56:32 -0400 Received: by mail-wr1-f67.google.com with SMTP id n14so1765110wrw.9 for ; Fri, 27 Sep 2019 01:56:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloud.ionos.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TfXue9T2NkQBattDkXEon2NvQgVQztlS6k0o1+O9NR0=; b=UjKJwYJryZcxHCXwQqzpWtx7u991HKb7WQkoTDphhyFcp+NE6J2M5rolmd9lkRTnXN C4AYPPAF8iVymbgKGXxwj1Js/d5pavwIOj5zRhLYh38nNpK4t1EwK0JmHHLubh2oGbBy D4YikOeHQ0nMpPAMhWGQ8uwv7xt1TrHHLCVouCe4bcD3wMu+cJnfNKjmbZPEhTf/zYQ1 EYj+BITqrZK9+eqJoOk1HeSsQRutPLl1R9sApgpiF9ya4c/EK9DKa/h2amlD+XNl9eUC U2rGO97uvHdABQ5+pYr/1tTIiACq9q+YdIiZhc/IDpqEGdU2ffc1OppOpgtoyMlZyESp 7AWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TfXue9T2NkQBattDkXEon2NvQgVQztlS6k0o1+O9NR0=; b=IRn/JFNnWa1E/R2S9oqMiZHrM7bKCD5Bf/zOepZBqOsLFGp4kzc8x2anlo7RQCb0Sk r+reKMaYaXOf4zbEFaZIIzBRBI084SY5TqgaaQ80k8KNyNhtq5Npip4eyoMC/Dri10rn EPAHwkQ82jIIFjI16G4B1wpLqoJPK1JieMYDClE1AYW9OADUY8ME3w5d3plrLO7rLNct dDQEhLpzQpaShAa0+cX82EAy420alGGtbIic07B6LgW7OnplOcxvuunzczLMMFlH82JQ IJJjzR0qUQXtS36Fsy/B/d6lvGzmSHHTVGwytfjLh81cZeuyKMuAnyMTVt8toShxPxzw ACnA== X-Gm-Message-State: APjAAAWCUqO/2R1mcNoUsrR8X1yaRoEWpHKQ3+FipHc6QnQGalC3Oti9 /hnyLmcxS+cbEdXolBy3vWkBkEtGav0PllxZGMr5jw== X-Google-Smtp-Source: APXvYqxfWyaZuRbOnAwrVlspvqQMOlUGBXujPtXA5C42+fyPB1kYHoqubhTogBMEJYCXzLDa7+1kLUmFpNdgFEhvi5k= X-Received: by 2002:adf:d192:: with SMTP id v18mr1043108wrc.9.1569574588074; Fri, 27 Sep 2019 01:56:28 -0700 (PDT) MIME-Version: 1.0 References: <20190620150337.7847-1-jinpuwang@gmail.com> <20190620150337.7847-4-jinpuwang@gmail.com> <7f62b16a-6e6c-ad05-46d4-05514ffaeaba@acm.org> In-Reply-To: <7f62b16a-6e6c-ad05-46d4-05514ffaeaba@acm.org> From: Jinpu Wang Date: Fri, 27 Sep 2019 10:56:17 +0200 Message-ID: Subject: Re: [PATCH v4 03/25] ibtrs: private headers with IBTRS protocol structs and helpers To: Bart Van Assche Cc: Jack Wang , linux-block@vger.kernel.org, linux-rdma@vger.kernel.org, Jens Axboe , Christoph Hellwig , Sagi Grimberg , Jason Gunthorpe , Doug Ledford , Danil Kipnis , rpenyaev@suse.de, Roman Pen Content-Type: text/plain; charset="UTF-8" Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org On Tue, Sep 24, 2019 at 12:50 AM Bart Van Assche wrote: > > 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. Sure, will do. > > > +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. We already checked the private_data_len on server side, see ibtrs_rdma_connect, and keep some reserved fields for future seems to be common practice for protocol, IMO. Also due to the fact, we already running the code in production, we want to keep the protocol compatible, so future transition could be smooth. > > > +/** > > + * 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. Ditto. > > > +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, ""); > > + pr_err("Invalid address family\n"); > > + return cnt; > > +} > > Since these functions are not in the hot path, please move these into a > .c file. ok. > > > +/** > > + * 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? This is needed for the compact layer, we redefine some FR functions to use FMR for our ConnectX2 X3 HCA. https://github.com/ionos-enterprise/ibnbd/tree/master/ibtrs/compat It will finally fade out, but it will take time. Thanks, Jinpu