From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DC8F1A0C45; Tue, 19 Oct 2021 03:15:28 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9BDBC40142; Tue, 19 Oct 2021 03:15:28 +0200 (CEST) Received: from mail-pg1-f177.google.com (mail-pg1-f177.google.com [209.85.215.177]) by mails.dpdk.org (Postfix) with ESMTP id EEEE34003E for ; Tue, 19 Oct 2021 03:15:27 +0200 (CEST) Received: by mail-pg1-f177.google.com with SMTP id 75so17852210pga.3 for ; Mon, 18 Oct 2021 18:15:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=kytU41Q/0js16mAc8cHvfkynSwjBqn5Ym5fURfDTl7g=; b=aiiF/Q5LfnkM6VvTbw7UyqmaV51+2eEAtRqWbQWPfFdS5oPhEgp+zf443vL3Dl0vvq ke74F2mV+18cQzO+Ihh0KQfb6oxsEpBbpGoHvYo0ILY2DKQpemirohSCFJge66bcPwWz QrMHfihhnVbitK0GcZCkVxU+2oL6ULZsxX/M7i643tsmAAgsG87FV7LbvshO05JSMHX1 sKydpe1w2UfIlhjpPmL+Eb8OhOUrzGeLEOwgpkwo10areSYRpP7pmN5k1AIkBJzbmaag alic5mpKOy3EFL+O9V8VZK2foQaKYP690q3z2BHVeSFwvOs8UF/vr/Zy0mHzJKWmS1wd Kgkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=kytU41Q/0js16mAc8cHvfkynSwjBqn5Ym5fURfDTl7g=; b=3rm3OnQuz/6HRtxBz4y3SyJ+JOHXAHUrtY9FcNjeeL6CZA3LvQZeVBfaG3HnR9l9jL F/8wvoyw4SK9JXOHsGLHJAkLkcDfzhlTkoKqZUF31WEBBDvTOPIX9jUxoMzWxTy5s+R/ qU6sbZsdPeKoVBg0FOwqKyybmGA3ltPnnQC3kcs2XwQ3Am4RAnVaW8kuS/uVi0ez/k2O UFr2vyVbL8JTBsJ60fI3xNhYMTRg8WhhHR2qAn/Hn0Np3l1qpGycTS6ti2fPramb5J6c iZIh43sUTcaoickPMHa9VsV3lEHkjek2mu7Et/Evh6fJwZOQ3RjtDMp4Zx4QQEFz/2TS btDA== X-Gm-Message-State: AOAM532gtJGJoAsps9DdiefNfHeCrW3vVcLcLvSstJ/IDfe1PRTFOg20 dVKjDajjF/KnOsLjmWPKcKbXXjnshAjFzw== X-Google-Smtp-Source: ABdhPJxGRNYB7TDpIWLSs0UhMi5NUha0rpHGQF2yiS2jCzBxmJ6GV2BlafeDHaSFyMVphRZ0ks2RzQ== X-Received: by 2002:a63:734c:: with SMTP id d12mr26423185pgn.358.1634606126767; Mon, 18 Oct 2021 18:15:26 -0700 (PDT) Received: from hermes.local (204-195-33-123.wavecable.com. [204.195.33.123]) by smtp.gmail.com with ESMTPSA id q35sm612386pjk.41.2021.10.18.18.15.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Oct 2021 18:15:26 -0700 (PDT) Date: Mon, 18 Oct 2021 18:15:23 -0700 From: Stephen Hemminger To: "Ananyev, Konstantin" Cc: "Medvedkin, Vladimir" , "dev@dpdk.org" , "Wang, Yipeng1" , "Gobriel, Sameh" , "Richardson, Bruce" Message-ID: <20211018181523.24f9657d@hermes.local> In-Reply-To: References: <1634290206-251913-1-git-send-email-vladimir.medvedkin@intel.com> <1634290206-251913-2-git-send-email-vladimir.medvedkin@intel.com> <20211015095834.469a4efd@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/5] hash: add new toeplitz hash implementation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, 18 Oct 2021 10:40:00 +0000 "Ananyev, Konstantin" wrote: > > On Fri, 15 Oct 2021 10:30:02 +0100 > > Vladimir Medvedkin wrote: > > > > > + m[i * 8 + j] = (rss_key[i] << j)| > > > + (uint8_t)((uint16_t)(rss_key[i + 1]) >> > > > + (8 - j)); > > > + } > > > > This ends up being harder than necessary to read. Maybe split into > > multiple statements and/or use temporary variable. > > > > > +RTE_INIT(rte_thash_gfni_init) > > > +{ > > > + rte_thash_gfni_supported = 0; > > > > Not necessary in C globals are initialized to zero by default. > > > > By removing that the constructor can be totally behind #ifdef > > > > > +__rte_internal > > > +static inline __m512i > > > +__rte_thash_gfni(const uint64_t *mtrx, const uint8_t *tuple, > > > + const uint8_t *secondary_tuple, int len) > > > +{ > > > + __m512i permute_idx = _mm512_set_epi8(7, 6, 5, 4, 7, 6, 5, 4, > > > + 6, 5, 4, 3, 6, 5, 4, 3, > > > + 5, 4, 3, 2, 5, 4, 3, 2, > > > + 4, 3, 2, 1, 4, 3, 2, 1, > > > + 3, 2, 1, 0, 3, 2, 1, 0, > > > + 2, 1, 0, -1, 2, 1, 0, -1, > > > + 1, 0, -1, -2, 1, 0, -1, -2, > > > + 0, -1, -2, -3, 0, -1, -2, -3); > > > > NAK > > > > Please don't put the implementation in an inline. This makes it harder > > to support (API/ABI) and blocks other architectures from implementing > > same thing with different instructions. > > I don't really understand your reasoning here. > rte_thash_gfni.h is an arch-specific header, which provides > arch-specific optimizations for RSS hash calculation > (Vladimir pls correct me if I am wrong here). Ok, but rte_thash_gfni.h is included on all architectures. > We do have dozens of inline functions that do use arch-specific instructions (both x86 and arm) > for different purposes: > sync primitives, memory-ordering, cache manipulations, LPM lookup, TSX, power-saving, etc. > That's a usual trade-off taken for performance reasons, when extra function call > costs too much comparing to the operation itself. > Why it suddenly became a problem for that particular case and how exactly it blocks other architectures? > Also I don't understand how it makes things harder in terms of API/ABI stability. > As I can see this patch doesn't introduce any public structs/unions. > All functions take as arguments just raw data buffers and length. > To summarize - in general, I don't see any good reason why this patch shouldn't be allowed. > Konstantin The comments about rte_thash_gfni_supported initialization still apply. Why not: #ifdef __GFNI__ RTE_INIT(rte_thash_gfni_init) { if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_GFNI)) rte_thash_gfni_supported = 1; } #endif