[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>
---
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 | 16 +++++
4 files changed, 101 insertions(+), 54 deletions(-)
Comments
On 3/27/20 1:17 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.
>
> 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>
[snip]
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index cab38152a7..4c1e4899d0 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -256,6 +256,22 @@ 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];
You have thread-safety patches in the series before this one.
Is it OK to be thread-unsafe here?
> +
> + RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
> +
> + if (unlikely(!key_generated)) {
I believe that unlikely() is not required here. It is not a
datapath and there is no point to use likely/unlikely on
control path.
> + for (size_t i = 0; i < ENA_HASH_KEY_SIZE; ++i)
It is C99 feature which breaks DPDK build pretty often, since
neither c99 nor higher are requested in default DPDK build.
> + 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)
> {
>
pt., 27 mar 2020 o 12:12 Andrew Rybchenko <arybchenko@solarflare.com>
napisał(a):
>
> On 3/27/20 1:17 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.
> >
> > 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>
>
> [snip]
>
> > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> > index cab38152a7..4c1e4899d0 100644
> > --- a/drivers/net/ena/ena_ethdev.c
> > +++ b/drivers/net/ena/ena_ethdev.c
> > @@ -256,6 +256,22 @@ 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];
>
> You have thread-safety patches in the series before this one.
> Is it OK to be thread-unsafe here?
>
> > +
> > + RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
> > +
> > + if (unlikely(!key_generated)) {
>
> I believe that unlikely() is not required here. It is not a
> datapath and there is no point to use likely/unlikely on
> control path.
>
I will remove it in v2.
> > + for (size_t i = 0; i < ENA_HASH_KEY_SIZE; ++i)
>
> It is C99 feature which breaks DPDK build pretty often, since
> neither c99 nor higher are requested in default DPDK build.
>
Ok, will be fixed in v2.
> > + 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)
> > {
> >
>
wt., 31 mar 2020 o 11:40 Michał Krawczyk <mk@semihalf.com> napisał(a):
>
> pt., 27 mar 2020 o 12:12 Andrew Rybchenko <arybchenko@solarflare.com>
> napisał(a):
> >
> > On 3/27/20 1:17 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.
> > >
> > > 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>
> >
> > [snip]
> >
> > > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> > > index cab38152a7..4c1e4899d0 100644
> > > --- a/drivers/net/ena/ena_ethdev.c
> > > +++ b/drivers/net/ena/ena_ethdev.c
> > > @@ -256,6 +256,22 @@ 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];
> >
> > You have thread-safety patches in the series before this one.
> > Is it OK to be thread-unsafe here?
> >
I forgot to refer to this comment, sorry. ena_rss_key_fill() is called
only from ena_start() and as far as I know, it can be called only from
single-threaded context (device configuration flow). In that case,
there is no risk and we can be thread-unsafe.
> > > +
> > > + RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
> > > +
> > > + if (unlikely(!key_generated)) {
> >
> > I believe that unlikely() is not required here. It is not a
> > datapath and there is no point to use likely/unlikely on
> > control path.
> >
>
> I will remove it in v2.
>
> > > + for (size_t i = 0; i < ENA_HASH_KEY_SIZE; ++i)
> >
> > It is C99 feature which breaks DPDK build pretty often, since
> > neither c99 nor higher are requested in default DPDK build.
> >
>
> Ok, will be fixed in v2.
>
> > > + 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)
> > > {
> > >
> >
@@ -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,22 @@ 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];
+
+ RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
+
+ if (unlikely(!key_generated)) {
+ for (size_t 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)
{