Re: [dpdk-dev] [PATCH v2 04/29] net/ena/base: set default hash key

["Michał Krawczyk" <mk@semihalf.com>]

czw., 2 kwi 2020 o 14:36 Ferruh Yigit <ferruh.yigit@intel.com> napisał(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 called
> 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 <mk@semihalf.com>
> > Reviewed-by: Igor Chauskin <igorch@amazon.com>
> > Reviewed-by: Guy Tzalik <gtzalik@amazon.com>
>
> <...>
>
> > @@ -1032,6 +1032,24 @@ static int ena_com_get_feature(struct ena_com_dev *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 function?
> Commit log mentions exposing to user, is the intention this function to be
> called by application, if so it shoudln't, applications doesn't call driver
> 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(struct 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] = { (u16)-1 };
> > -     struct ena_rss *rss = &ena_dev->rss;
> > -     u8 idx;
> > -     u16 i;
> > -
> > -     for (i = 0; i < ENA_TOTAL_NUM_QUEUES; i++)
> > -             dev_idx_to_host_tbl[ena_dev->io_sq_queues[i].idx] = i;
> > -
> > -     for (i = 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 = (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] = dev_idx_to_host_tbl[idx];
> > -     }
> > -
> > -     return 0;
> > -}> -
>
> This function and where it is called seems removed, is this related to this
> 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 != sizeof(hash_key->key)) {
> > +                             ena_trc_err("key len (%hu) doesn't equal the supported size (%zu)\n",
> > +                                          key_len, sizeof(hash_key->key));
> > +                             return ENA_COM_INVAL;
> > +                     }
> > +                     memcpy(hash_key->key, key, key_len);
> > +                     rss->hash_init_val = init_val;
> > +                     hash_key->keys_num = key_len >> 2;
>
> I am aware this is copy/paste, but here '>> 2' is "/ sizeof(unit2_t)", would 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 = {
> >       .reta_query           = 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' variables
> 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.