[v2,04/29] net/ena/base: set default hash key
Checks
Commit Message
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.
By default, the random hash key is used and it is generated only once
when requested for the first time.
Signed-off-by: Michal Krawczyk <mk@semihalf.com>
Reviewed-by: Igor Chauskin <igorch@amazon.com>
Reviewed-by: Guy Tzalik <gtzalik@amazon.com>
---
v2:
* Remove variable declaration inside the for loop
* Remove unlikely in condition check
drivers/net/ena/base/ena_com.c | 103 ++++++++++++++-------------
drivers/net/ena/base/ena_com.h | 30 ++++++--
drivers/net/ena/base/ena_plat_dpdk.h | 6 ++
drivers/net/ena/ena_ethdev.c | 17 +++++
4 files changed, 102 insertions(+), 54 deletions(-)
Comments
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'?
>
> 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?
>
> 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.
<...>
> @@ -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"?
<...>
> 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?
<...>
> @@ -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.
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.
@@ -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;
+}
+
+static void ena_com_hash_key_fill_default_key(struct ena_com_dev *ena_dev)
+{
+ struct ena_admin_feature_rss_flow_hash_control *hash_key =
+ (ena_dev->rss).hash_key;
+
+ ENA_RSS_FILL_KEY(&hash_key->key, sizeof(hash_key->key));
+ /* The key is stored in the device in uint32_t array
+ * as well as the API requires the key to be passed in this
+ * format. Thus the size of our array should be divided by 4
+ */
+ hash_key->keys_num = sizeof(hash_key->key) / sizeof(uint32_t);
+}
+
static int ena_com_hash_key_allocate(struct ena_com_dev *ena_dev)
{
struct ena_rss *rss = &ena_dev->rss;
@@ -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;
-}
-
static int ena_com_init_interrupt_moderation_table(struct ena_com_dev *ena_dev)
{
size_t size;
@@ -2381,12 +2375,14 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
enum ena_admin_hash_functions func,
const u8 *key, u16 key_len, u32 init_val)
{
- struct ena_rss *rss = &ena_dev->rss;
+ struct ena_admin_feature_rss_flow_hash_control *hash_key;
struct ena_admin_get_feat_resp get_resp;
- struct ena_admin_feature_rss_flow_hash_control *hash_key =
- rss->hash_key;
+ enum ena_admin_hash_functions old_func;
+ struct ena_rss *rss = &ena_dev->rss;
int rc;
+ hash_key = rss->hash_key;
+
/* Make sure size is a mult of DWs */
if (unlikely(key_len & 0x3))
return ENA_COM_INVAL;
@@ -2398,22 +2394,23 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
if (unlikely(rc))
return rc;
- if (!((1 << func) & get_resp.u.flow_hash_func.supported_func)) {
+ if (!(BIT(func) & get_resp.u.flow_hash_func.supported_func)) {
ena_trc_err("Flow hash function %d isn't supported\n", func);
return ENA_COM_UNSUPPORTED;
}
switch (func) {
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;
}
-
- memcpy(hash_key->key, key, key_len);
- rss->hash_init_val = init_val;
- hash_key->keys_num = key_len >> 2;
break;
case ENA_ADMIN_CRC32:
rss->hash_init_val = init_val;
@@ -2423,26 +2420,27 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
return ENA_COM_INVAL;
}
+ old_func = rss->hash_func;
rss->hash_func = func;
rc = ena_com_set_hash_function(ena_dev);
/* Restore the old function */
if (unlikely(rc))
- ena_com_get_hash_function(ena_dev, NULL, NULL);
+ rss->hash_func = old_func;
return rc;
}
int ena_com_get_hash_function(struct ena_com_dev *ena_dev,
- enum ena_admin_hash_functions *func,
- u8 *key)
+ enum ena_admin_hash_functions *func)
{
struct ena_rss *rss = &ena_dev->rss;
struct ena_admin_get_feat_resp get_resp;
- struct ena_admin_feature_rss_flow_hash_control *hash_key =
- rss->hash_key;
int rc;
+ if (unlikely(!func))
+ return ENA_COM_INVAL;
+
rc = ena_com_get_feature_ex(ena_dev, &get_resp,
ENA_ADMIN_RSS_HASH_FUNCTION,
rss->hash_key_dma_addr,
@@ -2450,9 +2448,20 @@ int ena_com_get_hash_function(struct ena_com_dev *ena_dev,
if (unlikely(rc))
return rc;
- rss->hash_func = get_resp.u.flow_hash_func.selected_func;
- if (func)
- *func = rss->hash_func;
+ /* ENA_FFS returns 1 in case the lsb is set */
+ rss->hash_func = ENA_FFS(get_resp.u.flow_hash_func.selected_func);
+ if (rss->hash_func)
+ rss->hash_func--;
+
+ *func = rss->hash_func;
+
+ return 0;
+}
+
+int ena_com_get_hash_key(struct ena_com_dev *ena_dev, u8 *key)
+{
+ struct ena_admin_feature_rss_flow_hash_control *hash_key =
+ ena_dev->rss.hash_key;
if (key)
memcpy(key, hash_key->key, (size_t)(hash_key->keys_num) << 2);
@@ -2714,10 +2723,6 @@ int ena_com_indirect_table_get(struct ena_com_dev *ena_dev, u32 *ind_tbl)
if (!ind_tbl)
return 0;
- rc = ena_com_ind_tbl_convert_from_device(ena_dev);
- if (unlikely(rc))
- return rc;
-
for (i = 0; i < (1 << rss->tbl_log_size); i++)
ind_tbl[i] = rss->host_rss_ind_tbl[i];
@@ -2738,6 +2743,8 @@ int ena_com_rss_init(struct ena_com_dev *ena_dev, u16 indr_tbl_log_size)
if (unlikely(rc))
goto err_hash_key;
+ ena_com_hash_key_fill_default_key(ena_dev);
+
rc = ena_com_hash_ctrl_init(ena_dev);
if (unlikely(rc))
goto err_hash_ctrl;
@@ -53,6 +53,7 @@
#define ENA_INTR_DELAY_NEW_VALUE_WEIGHT 4
#define ENA_INTR_MODER_LEVEL_STRIDE 1
#define ENA_INTR_BYTE_COUNT_NOT_SUPPORTED 0xFFFFFF
+#define ENA_HASH_KEY_SIZE 40
#define ENA_HW_HINTS_NO_TIMEOUT 0xFFFF
@@ -693,6 +694,14 @@ int ena_com_rss_init(struct ena_com_dev *ena_dev, u16 log_size);
*/
void ena_com_rss_destroy(struct ena_com_dev *ena_dev);
+/* ena_com_get_current_hash_function - Get RSS hash function
+ * @ena_dev: ENA communication layer struct
+ *
+ * Return the current hash function.
+ * @return: 0 or one of the ena_admin_hash_functions values.
+ */
+int ena_com_get_current_hash_function(struct ena_com_dev *ena_dev);
+
/* ena_com_fill_hash_function - Fill RSS hash function
* @ena_dev: ENA communication layer struct
* @func: The hash function (Toeplitz or crc)
@@ -724,13 +733,11 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
*/
int ena_com_set_hash_function(struct ena_com_dev *ena_dev);
-/* ena_com_get_hash_function - Retrieve the hash function and the hash key
- * from the device.
+/* ena_com_get_hash_function - Retrieve the hash function from the device.
* @ena_dev: ENA communication layer struct
* @func: hash function
- * @key: hash key
*
- * Retrieve the hash function and the hash key from the device.
+ * Retrieve the hash function from the device.
*
* @note: If the caller called ena_com_fill_hash_function but didn't flash
* it to the device, the new configuration will be lost.
@@ -738,9 +745,20 @@ int ena_com_set_hash_function(struct ena_com_dev *ena_dev);
* @return: 0 on Success and negative value otherwise.
*/
int ena_com_get_hash_function(struct ena_com_dev *ena_dev,
- enum ena_admin_hash_functions *func,
- u8 *key);
+ enum ena_admin_hash_functions *func);
+/* ena_com_get_hash_key - Retrieve the hash key
+ * @ena_dev: ENA communication layer struct
+ * @key: hash key
+ *
+ * Retrieve the hash key.
+ *
+ * @note: If the caller called ena_com_fill_hash_key but didn't flash
+ * it to the device, the new configuration will be lost.
+ *
+ * @return: 0 on Success and negative value otherwise.
+ */
+int ena_com_get_hash_key(struct ena_com_dev *ena_dev, u8 *key);
/* ena_com_fill_hash_ctrl - Fill RSS hash control
* @ena_dev: ENA communication layer struct.
* @proto: The protocol to configure.
@@ -301,6 +301,12 @@ extern rte_atomic32_t ena_alloc_cnt;
#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+#define ENA_FFS(x) ffs(x)
+
+void ena_rss_key_fill(void *key, size_t size);
+
+#define ENA_RSS_FILL_KEY(key, size) ena_rss_key_fill(key, size)
+
#include "ena_includes.h"
#endif /* DPDK_ENA_COM_ENA_PLAT_DPDK_H_ */
@@ -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];
+ size_t i;
+
+ RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
+
+ if (!key_generated) {
+ for (i = 0; i < ENA_HASH_KEY_SIZE; ++i)
+ default_key[i] = rte_rand() & 0xff;
+ key_generated = true;
+ }
+
+ rte_memcpy(key, default_key, size);
+}
+
static inline void ena_rx_mbuf_prepare(struct rte_mbuf *mbuf,
struct ena_com_rx_ctx *ena_rx_ctx)
{