From: Or Gerlitz <ogerlitz@mellanox•com>
To: Eyal Perry <eyalpe@mellanox•com>
Cc: Amir Vadai <amirv@mellanox•com>,
"David S. Miller" <davem@davemloft•net>,
Ben Hutchings <ben@decadent•org.uk>, <netdev@vger•kernel.org>,
"Yevgeny Petrilin" <yevgenyp@mellanox•com>
Subject: Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function
Date: Wed, 5 Nov 2014 14:27:46 +0200 [thread overview]
Message-ID: <545A17C2.9070609@mellanox.com> (raw)
In-Reply-To: <1415188769-19593-3-git-send-email-amirv@mellanox.com>
On 11/5/2014 1:59 PM, Amir Vadai wrote:
> From: Eyal Perry <eyalpe@mellanox•com>
>
> The ConnectX HW is capable of using one of the following hash functions:
> Toeplitz and an XOR hash function.
> This patch implements ethtool_ops {get,set}_rxfh_func callbacks for the
> mlx4_en driver in order to query and configure the RSS hash function to
> be used by the device.
>
> Signed-off-by: Eyal Perry <eyalpe@mellanox•com>
> Signed-off-by: Amir Vadai <amirv@mellanox•com>
> ---
> drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 40 +++++++++++++++++++++++++
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 9 ++++++
> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 11 ++++---
> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 3 +-
> 4 files changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 8ea4d5b..f33785a 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -973,6 +973,44 @@ static u32 mlx4_en_get_rxfh_indir_size(struct net_device *dev)
> return priv->rx_ring_num;
> }
>
> +static u32 mlx4_en_get_rxfh_func(struct net_device *dev)
> +{
> + struct mlx4_en_priv *priv = netdev_priv(dev);
> +
> + return priv->rss_hash_fn;
> +}
> +
> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
> +{
> + struct mlx4_en_priv *priv = netdev_priv(dev);
> + struct mlx4_en_dev *mdev = priv->mdev;
> + u32 prev_rss_hash_fn = priv->rss_hash_fn;
> + int err = 0;
> +
> + if (!(hfunc & priv->rss_hash_fn_caps))
> + return -EINVAL;
> +
> + priv->rss_hash_fn = hfunc;
Seems that you are blindly setting here priv->rss_hash_fn to the
function (xor or top) requested
by the user w.o prior checking if the firmware actually supports that
(e.g test it against priv->rss_hash_fn_caps, right?
> + if (hfunc == RSS_HASH_TOP && !(dev->features & NETIF_F_RXHASH))
> + en_warn(priv,
> + "Toeplitz hash function should be used in conjunction with RX hashing for optimal performance\n");
> + if (hfunc == RSS_HASH_XOR && (dev->features & NETIF_F_RXHASH))
> + en_warn(priv,
> + "Enabling both XOR Hash function and RX Hashing can limit RPS functionality\n");
> +
> + mutex_lock(&mdev->state_lock);
> + if (priv->port_up && priv->rss_hash_fn != prev_rss_hash_fn) {
> + mlx4_en_stop_port(dev, 1);
> + err = mlx4_en_start_port(dev);
> + }
> + mutex_unlock(&mdev->state_lock);
> +
> + if (err)
> + en_err(priv, "Failed to restart port %d\n", priv->port);
> +
> + return err;
> +}
> +
> static int mlx4_en_get_rxfh(struct net_device *dev, u32 *ring_index, u8 *key)
> {
> struct mlx4_en_priv *priv = netdev_priv(dev);
> @@ -1799,6 +1837,8 @@ const struct ethtool_ops mlx4_en_ethtool_ops = {
> .get_rxnfc = mlx4_en_get_rxnfc,
> .set_rxnfc = mlx4_en_set_rxnfc,
> .get_rxfh_indir_size = mlx4_en_get_rxfh_indir_size,
> + .get_rxfh_func = mlx4_en_get_rxfh_func,
> + .set_rxfh_func = mlx4_en_set_rxfh_func,
> .get_rxfh = mlx4_en_get_rxfh,
> .set_rxfh = mlx4_en_set_rxfh,
> .get_channels = mlx4_en_get_channels,
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 0efbae9..a9e96a6 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2585,6 +2585,15 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
> dev->features |= NETIF_F_GSO_UDP_TUNNEL;
> }
>
> + if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR) {
> + priv->rss_hash_fn_caps |= RSS_HASH_XOR;
> + priv->rss_hash_fn = RSS_HASH_XOR;
> + }
> + if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP) {
> + priv->rss_hash_fn_caps |= RSS_HASH_TOP;
> + priv->rss_hash_fn = RSS_HASH_TOP;
> + }
> +
> mdev->pndev[port] = dev;
>
> netif_carrier_off(dev);
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index b173a0c..41c1ff9 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct mlx4_en_priv *priv)
> }
>
> rss_context->flags = rss_mask;
> - rss_context->hash_fn = MLX4_RSS_HASH_TOP;
> - for (i = 0; i < 10; i++)
> - rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
> -
> + if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
> + rss_context->hash_fn = MLX4_RSS_HASH_XOR;
> + } else {
> + rss_context->hash_fn = MLX4_RSS_HASH_TOP;
> + for (i = 0; i < 10; i++)
> + rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
> + }
So this patch effectively changes the default from top to xor, right? is
that intentional? if yes, spare few words on the change log.
> err = mlx4_qp_to_ready(mdev->dev, &priv->res.mtt, &context,
> &rss_map->indir_qp, &rss_map->indir_state);
> if (err)
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> index ef83d12..e1c3364 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> @@ -375,7 +375,6 @@ struct mlx4_en_port_profile {
> };
>
> struct mlx4_en_profile {
> - int rss_xor;
> int udp_rss;
> u8 rss_mask;
> u32 active_ports;
> @@ -615,6 +614,8 @@ struct mlx4_en_priv {
> __be16 vxlan_port;
>
> u32 pflags;
> + u8 rss_hash_fn;
> + u8 rss_hash_fn_caps;
> };
>
> enum mlx4_en_wol {
next prev parent reply other threads:[~2014-11-05 12:29 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-05 11:59 [PATCH net-next 0/2] ethtool, net/mlx4_en: RSS hash function selection Amir Vadai
2014-11-05 11:59 ` [PATCH net-next 1/2] ethtool: Support for configurable RSS hash function Amir Vadai
2014-11-05 21:51 ` Ben Hutchings
2014-11-06 9:41 ` Eyal perry
2014-11-05 11:59 ` [PATCH net-next 2/2] net/mlx4_en: " Amir Vadai
2014-11-05 12:27 ` Or Gerlitz [this message]
2014-11-05 13:05 ` Eyal perry
2014-11-05 13:30 ` Or Gerlitz
2014-11-05 13:48 ` Eyal perry
2014-11-05 13:53 ` Or Gerlitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=545A17C2.9070609@mellanox.com \
--to=ogerlitz@mellanox$(echo .)com \
--cc=amirv@mellanox$(echo .)com \
--cc=ben@decadent$(echo .)org.uk \
--cc=davem@davemloft$(echo .)net \
--cc=eyalpe@mellanox$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=yevgenyp@mellanox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox