From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E870CA0562; Fri, 3 Apr 2020 13:10:15 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 497261BEC7; Fri, 3 Apr 2020 13:10:15 +0200 (CEST) Received: from mail-ed1-f68.google.com (mail-ed1-f68.google.com [209.85.208.68]) by dpdk.org (Postfix) with ESMTP id 102562B89 for ; Fri, 3 Apr 2020 13:10:13 +0200 (CEST) Received: by mail-ed1-f68.google.com with SMTP id cw6so8769026edb.9 for ; Fri, 03 Apr 2020 04:10:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=I4buEfcBlUo8CGN9v9OXRoZTN92myeo33ku1a5uD0cM=; b=qEHW0anWC/VQ34AEsERuMqDkzLGNGmHl5zlAybCcgK5GmfhHMv9Ap9mJ7VVcyVSf2Z I2bv8KfMUdu3CGZaSjfrZGOR400o7tFHFTdIpbi9pGYPKXyOTmm4oEf/IpamhcUEhHlG r28SOR35BOaTaWSPzka5Gn1gg15J2QrT1XBZYs1k5TPnReMCCEofvlq8QjsNfFZt/3CG g5OafG3Z3UTKU/Ykv8DOIe4v5t1tlVslAWSgQ0mpmXn0cvr71BuDj7Lk1d4+3dAYP1xs 6X5DOow8c/TtInYEJDAmM6iWy4sLUwPc8xYiX7Qiq5ZmOtVWOOBUN5bKD6I3sPgw8lLh 12pg== 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:content-transfer-encoding; bh=I4buEfcBlUo8CGN9v9OXRoZTN92myeo33ku1a5uD0cM=; b=d1KsJNdG94bhYKB5cQDypir6wVG4160hmtp66/dRsv9lVI9aPBsb6WAqb2DAiUd0ly 5+kx7RiOeatt8gFAmJUSl8yi6jiABjDXZ73J2ZlPedzw8Fxj9swm41+a2k6kMADmfzHH nYT6ZOX5bLcr29ZG1jkp9VgoEkv2kvuEiWQQ9R/OM36Mwp9/omTBOvMeLfAw3gbfM/zG beZNC1S42ac1d+ESf16hCKuOd1z++uSSSTzc5wnkvPNsntKbOfBOH8M+4SepOTXxcasR RG9Xzto9icdC1BpOat5rSbw8ssA0yTfl7Q3NC8hIUOMsvgztoFk+FYZVurXxuD3e6rAK gfJA== X-Gm-Message-State: AGi0PubV3QaNRIp5O/5pWZuK03HPocEVypl6NqTR1fxPZwClLTTNHBvy LTmQN0KMK/d1fw5I2y41Un9+exafmOGF22qe/RkvlA== X-Google-Smtp-Source: APiQypLdUJoL9ihd7MSMSOqynO+aAvR3a3zn14Wcq7DE6J5EaFRe3cM8B89KGIv6U5KLEi375PvwWKybzvNcSy1rmbo= X-Received: by 2002:a17:906:164f:: with SMTP id n15mr7440552ejd.322.1585912212653; Fri, 03 Apr 2020 04:10:12 -0700 (PDT) MIME-Version: 1.0 References: <20200401142127.13715-1-mk@semihalf.com> <20200401142127.13715-5-mk@semihalf.com> In-Reply-To: From: =?UTF-8?Q?Micha=C5=82_Krawczyk?= Date: Fri, 3 Apr 2020 13:10:00 +0200 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org, Marcin Wojtas , Maciej Bielski , "Tzalik, Guy" , "Schmeilin, Evgeny" , "Chauskin, Igor" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v2 04/29] net/ena/base: set default hash key X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" czw., 2 kwi 2020 o 14:36 Ferruh Yigit napisa=C5=82= (a): > > On 4/1/2020 3:21 PM, Michal Krawczyk wrote: > > The RSS hash key was present in the device, but it wasn't exposed to > > the user. The other key still cannot be set, but now it can be accessed > > if one needs to do that. > > What is 'the other key'? > I meant 'the custom key provided by the user'. I'll precise this in v3. > > > > By default, the random hash key is used and it is generated only once > > when requested for the first time. > > It looks like this patch does > 1- setting default rss hash key (generated randomly) > 2- Separate 'ena_com_get_hash_function()' and 'ena_com_get_hash_key()' > - I didn't get the motivation of this seperation, new function not call= ed > 3- Minor fixes, like 'key' check in 'ena_com_fill_hash_function()', in > 'ENA_ADMIN_TOEPLITZ' case, ... > 4- remove 'ena_com_ind_tbl_convert_from_device()' > > What to you think seperating above ones if they are logically seperate? > Sure, I'll split this into 3 commits - I'll skip the 2nd one as initially we were planning to add full RSS configuration support in this release, but postponed it as for now (it's a really big patch). So this new function (ena_com_get_hash_key()) will be used there, but maybe as it's not yet in, we can skip this. > > > > Signed-off-by: Michal Krawczyk > > Reviewed-by: Igor Chauskin > > Reviewed-by: Guy Tzalik > > <...> > > > @@ -1032,6 +1032,24 @@ static int ena_com_get_feature(struct ena_com_de= v *ena_dev, > > feature_ver); > > } > > > > +int ena_com_get_current_hash_function(struct ena_com_dev *ena_dev) > > +{ > > + return ena_dev->rss.hash_func; > > +} > > > This function is not called, who is the intendent consumer of the functio= n? > Commit log mentions exposing to user, is the intention this function to b= e > called by application, if so it shoudln't, applications doesn't call driv= er > fucntions directly. > The intended customer was the PMD, which could further expose it to the application via DPDK API - but we're missing this feature in this release, so maybe it'll be better to add it later. > <...> > > > @@ -1266,30 +1284,6 @@ static int ena_com_ind_tbl_convert_to_device(str= uct ena_com_dev *ena_dev) > > return 0; > > } > > > > -static int ena_com_ind_tbl_convert_from_device(struct ena_com_dev *ena= _dev) > > -{ > > - u16 dev_idx_to_host_tbl[ENA_TOTAL_NUM_QUEUES] =3D { (u16)-1 }; > > - struct ena_rss *rss =3D &ena_dev->rss; > > - u8 idx; > > - u16 i; > > - > > - for (i =3D 0; i < ENA_TOTAL_NUM_QUEUES; i++) > > - dev_idx_to_host_tbl[ena_dev->io_sq_queues[i].idx] =3D i; > > - > > - for (i =3D 0; i < 1 << rss->tbl_log_size; i++) { > > - if (rss->rss_ind_tbl[i].cq_idx > ENA_TOTAL_NUM_QUEUES) > > - return ENA_COM_INVAL; > > - idx =3D (u8)rss->rss_ind_tbl[i].cq_idx; > > - > > - if (dev_idx_to_host_tbl[idx] > ENA_TOTAL_NUM_QUEUES) > > - return ENA_COM_INVAL; > > - > > - rss->host_rss_ind_tbl[i] =3D dev_idx_to_host_tbl[idx]; > > - } > > - > > - return 0; > > -}> - > > This function and where it is called seems removed, is this related to th= is > patch, "setting default hash key"? > Yeah - it looks like it should be a part of the separate patch. > <...> > > > case ENA_ADMIN_TOEPLITZ: > > - if (key_len > sizeof(hash_key->key)) { > > - ena_trc_err("key len (%hu) is bigger than the max= supported (%zu)\n", > > - key_len, sizeof(hash_key->key)); > > - return ENA_COM_INVAL; > > + if (key) { > > + if (key_len !=3D sizeof(hash_key->key)) { > > + ena_trc_err("key len (%hu) doesn't equal = the supported size (%zu)\n", > > + key_len, sizeof(hash_key->ke= y)); > > + return ENA_COM_INVAL; > > + } > > + memcpy(hash_key->key, key, key_len); > > + rss->hash_init_val =3D init_val; > > + hash_key->keys_num =3D key_len >> 2; > > I am aware this is copy/paste, but here '>> 2' is "/ sizeof(unit2_t)", wo= uld it > be better to use that way instead of hardcoded value? > Will change that in v3. > <...> > > > @@ -256,6 +256,23 @@ static const struct eth_dev_ops ena_dev_ops =3D { > > .reta_query =3D ena_rss_reta_query, > > }; > > > > +void ena_rss_key_fill(void *key, size_t size) > > +{ > > + static bool key_generated; > > + static uint8_t default_key[ENA_HASH_KEY_SIZE]; > > If there are multiple 'ena' devices in the platform, using 'static' varia= bles > will cause each device use same rss key, I just want to double check this= is the > intention. Yes, it's acceptable behavior. The most important thing is to have those keys random per instance, to make the DDoS attacks harder.