diff mbox series

[V1,net-next,02/13] net: ena: avoid unnecessary admin command when RSS function set fails

Message ID 20200422081628.8103-3-sameehj@amazon.com
State Changes Requested
Delegated to: David Miller
Headers show
Series Enhance current features in ena driver | expand

Commit Message

Jubran, Samih April 22, 2020, 8:16 a.m. UTC 
From: Arthur Kiyanovski <akiyano@amazon.com>

Currently when ena_set_hash_function() fails the hash function is
restored to the previous value by calling an admin command to get
the hash function from the device.

In this commit we avoid the admin command, by saving the previous
hash function before calling ena_set_hash_function() and using this
previous value to restore the hash function in case of failure of
ena_set_hash_function().

Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski April 23, 2020, 12:39 a.m. UTC | #1 
On Wed, 22 Apr 2020 08:16:17 +0000 sameehj@amazon.com wrote:
> From: Arthur Kiyanovski <akiyano@amazon.com>
> 
> Currently when ena_set_hash_function() fails the hash function is
> restored to the previous value by calling an admin command to get
> the hash function from the device.

I don't see how. func argument is NULL. If I'm reading the code right
if this function is restoring anything it's restoring the rss key.

> In this commit we avoid the admin command, by saving the previous
> hash function before calling ena_set_hash_function() and using this
> previous value to restore the hash function in case of failure of
> ena_set_hash_function().
> 
> Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_com.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 07b0f396d3c2..66edc86c41c9 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -2286,6 +2286,7 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
>  	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;
>  	int rc;
>  
>  	/* Make sure size is a mult of DWs */
> @@ -2325,12 +2326,13 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
>  		return -EINVAL;
>  	}
>  
> +	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;

Please order your commits correctly.

>  	return rc;
>  }
Jakub Kicinski April 23, 2020, 12:42 a.m. UTC | #2 
On Wed, 22 Apr 2020 17:39:41 -0700 Jakub Kicinski wrote:
> On Wed, 22 Apr 2020 08:16:17 +0000 sameehj@amazon.com wrote:
> > From: Arthur Kiyanovski <akiyano@amazon.com>
> > 
> > Currently when ena_set_hash_function() fails the hash function is
> > restored to the previous value by calling an admin command to get
> > the hash function from the device.  
> 
> I don't see how. func argument is NULL. If I'm reading the code right
> if this function is restoring anything it's restoring the rss key.

Ah, my bad, it sets the function in dev->rss, too.

Please reorganize these changes, they are very poorly split up right
now.

> > In this commit we avoid the admin command, by saving the previous
> > hash function before calling ena_set_hash_function() and using this
> > previous value to restore the hash function in case of failure of
> > ena_set_hash_function().
> > 
> > Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
> > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> > ---
> >  drivers/net/ethernet/amazon/ena/ena_com.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> > index 07b0f396d3c2..66edc86c41c9 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> > @@ -2286,6 +2286,7 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
> >  	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;
> >  	int rc;
> >  
> >  	/* Make sure size is a mult of DWs */
> > @@ -2325,12 +2326,13 @@ int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
> >  		return -EINVAL;
> >  	}
> >  
> > +	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;  
> 
> Please order your commits correctly.
> 
> >  	return rc;
> >  }
Jubran, Samih April 26, 2020, 9:45 a.m. UTC | #3 
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, April 23, 2020 3:43 AM
> To: Jubran, Samih <sameehj@amazon.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Kiyanovski, Arthur
> <akiyano@amazon.com>; Woodhouse, David <dwmw@amazon.co.uk>;
> Machulsky, Zorik <zorik@amazon.com>; Matushevsky, Alexander
> <matua@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; Wilson,
> Matt <msw@amazon.com>; Liguori, Anthony <aliguori@amazon.com>;
> Bshara, Nafea <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>;
> Belgazal, Netanel <netanel@amazon.com>; Saidi, Ali
> <alisaidi@amazon.com>; Herrenschmidt, Benjamin <benh@amazon.com>;
> Dagan, Noam <ndagan@amazon.com>
> Subject: RE: [EXTERNAL] [PATCH V1 net-next 02/13] net: ena: avoid
> unnecessary admin command when RSS function set fails
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On Wed, 22 Apr 2020 17:39:41 -0700 Jakub Kicinski wrote:
> > On Wed, 22 Apr 2020 08:16:17 +0000 sameehj@amazon.com wrote:
> > > From: Arthur Kiyanovski <akiyano@amazon.com>
> > >
> > > Currently when ena_set_hash_function() fails the hash function is
> > > restored to the previous value by calling an admin command to get
> > > the hash function from the device.
> >
> > I don't see how. func argument is NULL. If I'm reading the code right
> > if this function is restoring anything it's restoring the rss key.
> 
> Ah, my bad, it sets the function in dev->rss, too.
> 
> Please reorganize these changes, they are very poorly split up right now.
> 
> > > In this commit we avoid the admin command, by saving the previous
> > > hash function before calling ena_set_hash_function() and using this
> > > previous value to restore the hash function in case of failure of
> > > ena_set_hash_function().
> > >
> > > Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
> > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> > > ---
> > >  drivers/net/ethernet/amazon/ena/ena_com.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
> > > b/drivers/net/ethernet/amazon/ena/ena_com.c
> > > index 07b0f396d3c2..66edc86c41c9 100644
> > > --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> > > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> > > @@ -2286,6 +2286,7 @@ int ena_com_fill_hash_function(struct
> ena_com_dev *ena_dev,
> > >     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;
> > >     int rc;
> > >
> > >     /* Make sure size is a mult of DWs */ @@ -2325,12 +2326,13 @@
> > > int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
> > >             return -EINVAL;
> > >     }
> > >
> > > +   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;
> >
> > Please order your commits correctly.
You are right, the order of the commits is not ideal, will fix in v2.
> >
> > >     return rc;
> > >  }
diff mbox series

Patch

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 07b0f396d3c2..66edc86c41c9 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -2286,6 +2286,7 @@  int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
 	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;
 	int rc;
 
 	/* Make sure size is a mult of DWs */
@@ -2325,12 +2326,13 @@  int ena_com_fill_hash_function(struct ena_com_dev *ena_dev,
 		return -EINVAL;
 	}
 
+	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;
 }