diff mbox series

[net-next] nfp: VF rate limit support

Message ID 20220422131945.948311-1-simon.horman@corigine.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] nfp: VF rate limit support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 6 maintainers not CCed: pabeni@redhat.com yinjun.zhang@corigine.com niklas.soderlund@corigine.com baowen.zheng@corigine.com louis.peens@corigine.com fei.qin@corigine.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Simon Horman April 22, 2022, 1:19 p.m. UTC 
From: Bin Chen <bin.chen@corigine.com>

This patch enhances the NFP driver to supports assignment of
both max_tx_rate and min_tx_rate to VFs

The following configurations are all supported:
 # ip link set $DEV vf $VF_NUM max_tx_rate $RATE_VALUE
 # ip link set $DEV vf $VF_NUM min_tx_rate $RATE_VALUE
 # ip link set $DEV vf $VF_NUM max_tx_rate $RATE_VALUE \
			       min_tx_rate $RATE_VALUE
 # ip link set $DEV vf $VF_NUM min_tx_rate $RATE_VALUE \
			       max_tx_rate $RATE_VALUE

The max RATE_VALUE is limited to 0xFFFF which is about
63Gbps (using 1024 for 1G).

Signed-off-by: Bin Chen <bin.chen@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../ethernet/netronome/nfp/nfp_net_common.c   |  1 +
 .../ethernet/netronome/nfp/nfp_net_sriov.c    | 50 ++++++++++++++++++-
 .../ethernet/netronome/nfp/nfp_net_sriov.h    |  9 ++++
 3 files changed, 58 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski April 25, 2022, 11:53 p.m. UTC | #1 
On Fri, 22 Apr 2022 15:19:45 +0200 Simon Horman wrote:
> +	if (max_tx_rate > 0 || min_tx_rate > 0) {
> +		if (max_tx_rate > 0 && max_tx_rate < min_tx_rate) {
> +			nfp_warn(app->cpp, "min-tx-rate exceeds max_tx_rate.\n");
> +			return -EINVAL;
> +		}

This check should be moved to the core, I reckon.

> +		if (max_tx_rate > NFP_NET_VF_RATE_MAX || min_tx_rate > NFP_NET_VF_RATE_MAX) {

Please wrap the lines at 80 chars, it's actually going to be easier 
to read here.

> +			nfp_warn(app->cpp, "tx-rate exceeds 0x%x.\n", NFP_NET_VF_RATE_MAX);

Does it really make sense to print the rate as hex?

> +			return -EINVAL;
> +		}

> @@ -261,5 +294,18 @@ int nfp_app_get_vf_config(struct net_device *netdev, int vf,
>  	ivi->trusted = FIELD_GET(NFP_NET_VF_CFG_CTRL_TRUST, flags);
>  	ivi->linkstate = FIELD_GET(NFP_NET_VF_CFG_CTRL_LINK_STATE, flags);
>  
> +	err = nfp_net_sriov_check(app, vf, NFP_NET_VF_CFG_MB_CAP_RATE, "rate");
> +	if (!err) {
> +		rate = readl(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_RATE);
> +
> +		ivi->max_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MAX_RATE, rate);
> +		ivi->min_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MIN_RATE, rate);
> +
> +		if (ivi->max_tx_rate == NFP_NET_VF_RATE_MAX)
> +			ivi->max_tx_rate = 0;

If rate == NFP_NET_VF_RATE_MAX means unset then the check on set should
disallow it, IOW:

	if (max_tx_rate >= NFP_NET_VF_RATE_MAX || 
            min_tx_rate >= NFP_NET_VF_RATE_MAX) {
		nfp_war(...

no?

> +		if (ivi->min_tx_rate == NFP_NET_VF_RATE_MAX)
> +			ivi->max_tx_rate = 0;

*squint* you check min and clear max, is this intentional?
Bin Chen April 29, 2022, 8:54 a.m. UTC | #2 
On Tue, 26 Apr 2022 7:53 AM, Jakub wrote:
> On Fri, 22 Apr 2022 15:19:45 +0200 Simon Horman wrote:
> > +	if (max_tx_rate > 0 || min_tx_rate > 0) {
> > +		if (max_tx_rate > 0 && max_tx_rate < min_tx_rate) {
> > +			nfp_warn(app->cpp, "min-tx-rate exceeds max_tx_rate.\n");
> > +			return -EINVAL;
> > +		}
> 
> This check should be moved to the core, I reckon.
>
We agree with your suggestion, thanks. We plan to do this in two steps:
1.The firmware that currently support this feature will reject the nonzero min_tx_rate configuration, so the check here will not step in.  We will remove the check from driver site and upstream the patch. 
2.We will do more investigation jobs and add an appropriate check in the core.
What do you think?

> > +		if (max_tx_rate > NFP_NET_VF_RATE_MAX || min_tx_rate >
> > +NFP_NET_VF_RATE_MAX) {
> 
> Please wrap the lines at 80 chars, it's actually going to be easier to read here.
Accepted. Thank you. 

> 
> > +			nfp_warn(app->cpp, "tx-rate exceeds 0x%x.\n", 
> > +NFP_NET_VF_RATE_MAX);
> 
> Does it really make sense to print the rate as hex?
> 
> > +			return -EINVAL;
> > +		}
> 
Accepted. I will use "%d" instead.


> > @@ -261,5 +294,18 @@ int nfp_app_get_vf_config(struct net_device
> *netdev, int vf,
> >  	ivi->trusted = FIELD_GET(NFP_NET_VF_CFG_CTRL_TRUST, flags);
> >  	ivi->linkstate = FIELD_GET(NFP_NET_VF_CFG_CTRL_LINK_STATE, flags);
> >
> > +	err = nfp_net_sriov_check(app, vf, NFP_NET_VF_CFG_MB_CAP_RATE,
> "rate");
> > +	if (!err) {
> > +		rate = readl(app->pf->vfcfg_tbl2 + vf_offset + 
> > +NFP_NET_VF_CFG_RATE);
> > +
> > +		ivi->max_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MAX_RATE, rate);
> > +		ivi->min_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MIN_RATE, rate);
> > +
> > +		if (ivi->max_tx_rate == NFP_NET_VF_RATE_MAX)
> > +			ivi->max_tx_rate = 0;
> 
> If rate == NFP_NET_VF_RATE_MAX means unset then the check on set 
> should disallow it, IOW:
> 
> 	if (max_tx_rate >= NFP_NET_VF_RATE_MAX ||
>             min_tx_rate >= NFP_NET_VF_RATE_MAX) {
> 		nfp_war(...
> 
> no?
>
Yes, there is a bug，thank you for catching it. I will enhance the check on set as your suggestion.


> > +		if (ivi->min_tx_rate == NFP_NET_VF_RATE_MAX)
> > +			ivi->max_tx_rate = 0;
> 
> *squint* you check min and clear max, is this intentional?
It is not intentional to do, it must be an accident. I will fix it. Thank you.

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2022.4.6 7:53
> To: Simon Horman <simon.horman@corigine.com>
> Cc: David Miller <davem@davemloft.net>; netdev@vger.kernel.org; oss-
> drivers <oss-drivers@corigine.com>; Bin Chen <bin.chen@corigine.com>
> Subject: Re: [PATCH net-next] nfp: VF rate limit support
> 
> On Fri, 22 Apr 2022 15:19:45 +0200 Simon Horman wrote:
> > +	if (max_tx_rate > 0 || min_tx_rate > 0) {
> > +		if (max_tx_rate > 0 && max_tx_rate < min_tx_rate) {
> > +			nfp_warn(app->cpp, "min-tx-rate exceeds
> max_tx_rate.\n");
> > +			return -EINVAL;
> > +		}
> 
> This check should be moved to the core, I reckon.
> 
> > +		if (max_tx_rate > NFP_NET_VF_RATE_MAX || min_tx_rate >
> > +NFP_NET_VF_RATE_MAX) {
> 
> Please wrap the lines at 80 chars, it's actually going to be easier to read here.
> 
> > +			nfp_warn(app->cpp, "tx-rate exceeds 0x%x.\n",
> > +NFP_NET_VF_RATE_MAX);
> 
> Does it really make sense to print the rate as hex?
> 
> > +			return -EINVAL;
> > +		}
> 
> > @@ -261,5 +294,18 @@ int nfp_app_get_vf_config(struct net_device
> *netdev, int vf,
> >  	ivi->trusted = FIELD_GET(NFP_NET_VF_CFG_CTRL_TRUST, flags);
> >  	ivi->linkstate = FIELD_GET(NFP_NET_VF_CFG_CTRL_LINK_STATE,
> flags);
> >
> > +	err = nfp_net_sriov_check(app, vf,
> NFP_NET_VF_CFG_MB_CAP_RATE, "rate");
> > +	if (!err) {
> > +		rate = readl(app->pf->vfcfg_tbl2 + vf_offset +
> > +NFP_NET_VF_CFG_RATE);
> > +
> > +		ivi->max_tx_rate =
> FIELD_GET(NFP_NET_VF_CFG_MAX_RATE, rate);
> > +		ivi->min_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MIN_RATE,
> rate);
> > +
> > +		if (ivi->max_tx_rate == NFP_NET_VF_RATE_MAX)
> > +			ivi->max_tx_rate = 0;
> 
> If rate == NFP_NET_VF_RATE_MAX means unset then the check on set
> should disallow it, IOW:
> 
> 	if (max_tx_rate >= NFP_NET_VF_RATE_MAX ||
>             min_tx_rate >= NFP_NET_VF_RATE_MAX) {
> 		nfp_war(...
> 
> no?
> 
> > +		if (ivi->min_tx_rate == NFP_NET_VF_RATE_MAX)
> > +			ivi->max_tx_rate = 0;
> 
> *squint* you check min and clear max, is this intentional?
Jakub Kicinski April 29, 2022, 6:03 p.m. UTC | #3 
On Fri, 29 Apr 2022 08:54:53 +0000 Bin Chen wrote:
> On Tue, 26 Apr 2022 7:53 AM, Jakub wrote:
> > On Fri, 22 Apr 2022 15:19:45 +0200 Simon Horman wrote:  
> > > +	if (max_tx_rate > 0 || min_tx_rate > 0) {
> > > +		if (max_tx_rate > 0 && max_tx_rate < min_tx_rate) {
> > > +			nfp_warn(app->cpp, "min-tx-rate exceeds max_tx_rate.\n");
> > > +			return -EINVAL;
> > > +		}  
> > 
> > This check should be moved to the core, I reckon.
> >  
> We agree with your suggestion, thanks. We plan to do this in two steps:
> 1.The firmware that currently support this feature will reject the nonzero min_tx_rate configuration, so the check here will not step in.  We will remove the check from driver site and upstream the patch. 
> 2.We will do more investigation jobs and add an appropriate check in the core.
> What do you think?

Sorry, I meant the second part of the condition only, basically
something like:
Jakub Kicinski April 29, 2022, 6:04 p.m. UTC | #4 
On Fri, 29 Apr 2022 11:03:47 -0700 Jakub Kicinski wrote:
> On Fri, 29 Apr 2022 08:54:53 +0000 Bin Chen wrote:
> > We agree with your suggestion, thanks. We plan to do this in two steps:
> > 1.The firmware that currently support this feature will reject the nonzero min_tx_rate configuration, so the check here will not step in.  We will remove the check from driver site and upstream the patch. 
> > 2.We will do more investigation jobs and add an appropriate check in the core.
> > What do you think?  
> 
> Sorry, I meant the second part of the condition only, basically
> something like:

I hit the wrong shortcut :) Here's the patch:

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 73f2cbc440c9..8de191cedaf7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2368,6 +2368,19 @@ static int handle_vf_guid(struct net_device *dev, struct ifla_vf_guid *ivt, int
 	return handle_infiniband_guid(dev, ivt, guid_type);
 }
 
+static int rtnl_set_vf_rate(struct net_device *dev, int vf, int min_tx_rate,
+			    int max_tx_rate)
+{
+	int err;
+
+	if (!ops->ndo_set_vf_rate)
+		return -EOPNOTSUPP;
+	if (min_tx_rate > max_tx_rate)
+		return -EINVAL;
+
+	return ops->ndo_set_vf_rate(dev, vf, min_tx_rate, max_tx_rate);
+}
+
 static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
@@ -2443,11 +2456,8 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 		if (err < 0)
 			return err;
 
-		err = -EOPNOTSUPP;
-		if (ops->ndo_set_vf_rate)
-			err = ops->ndo_set_vf_rate(dev, ivt->vf,
-						   ivf.min_tx_rate,
-						   ivt->rate);
+		err = rtnl_set_vf_rate(dev, ivt->vf,
+				       ivf.min_tx_rate, ivt->rate);
 		if (err < 0)
 			return err;
 	}
@@ -2457,11 +2467,8 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 
 		if (ivt->vf >= INT_MAX)
 			return -EINVAL;
-		err = -EOPNOTSUPP;
-		if (ops->ndo_set_vf_rate)
-			err = ops->ndo_set_vf_rate(dev, ivt->vf,
-						   ivt->min_tx_rate,
-						   ivt->max_tx_rate);
+		err = rtnl_set_vf_rate(dev, ivt->vf,
+				       ivt->min_tx_rate, ivt->max_tx_rate);
 		if (err < 0)
 			return err;
 	}
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index b412670d89b2..4340b69cc919 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -1903,6 +1903,7 @@  const struct net_device_ops nfp_nfd3_netdev_ops = {
 	.ndo_vlan_rx_kill_vid	= nfp_net_vlan_rx_kill_vid,
 	.ndo_set_vf_mac         = nfp_app_set_vf_mac,
 	.ndo_set_vf_vlan        = nfp_app_set_vf_vlan,
+	.ndo_set_vf_rate	= nfp_app_set_vf_rate,
 	.ndo_set_vf_spoofchk    = nfp_app_set_vf_spoofchk,
 	.ndo_set_vf_trust	= nfp_app_set_vf_trust,
 	.ndo_get_vf_config	= nfp_app_get_vf_config,
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
index 4627715a5e32..bca0a864cb44 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
@@ -142,6 +142,40 @@  int nfp_app_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, u8 qos,
 	return nfp_net_sriov_update(app, vf, update, "vlan");
 }
 
+int nfp_app_set_vf_rate(struct net_device *netdev, int vf,
+			int min_tx_rate, int max_tx_rate)
+{
+	struct nfp_app *app = nfp_app_from_netdev(netdev);
+	u32 vf_offset, ratevalue;
+	int err;
+
+	err = nfp_net_sriov_check(app, vf, NFP_NET_VF_CFG_MB_CAP_RATE, "rate");
+	if (err)
+		return err;
+
+	if (max_tx_rate > 0 || min_tx_rate > 0) {
+		if (max_tx_rate > 0 && max_tx_rate < min_tx_rate) {
+			nfp_warn(app->cpp, "min-tx-rate exceeds max_tx_rate.\n");
+			return -EINVAL;
+		}
+
+		if (max_tx_rate > NFP_NET_VF_RATE_MAX || min_tx_rate > NFP_NET_VF_RATE_MAX) {
+			nfp_warn(app->cpp, "tx-rate exceeds 0x%x.\n", NFP_NET_VF_RATE_MAX);
+			return -EINVAL;
+		}
+	}
+
+	vf_offset = NFP_NET_VF_CFG_MB_SZ + vf * NFP_NET_VF_CFG_SZ;
+	ratevalue = FIELD_PREP(NFP_NET_VF_CFG_MAX_RATE,
+			       max_tx_rate ? max_tx_rate : NFP_NET_VF_RATE_MAX) |
+		    FIELD_PREP(NFP_NET_VF_CFG_MIN_RATE, min_tx_rate);
+
+	writel(ratevalue, app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_RATE);
+
+	return nfp_net_sriov_update(app, vf, NFP_NET_VF_CFG_MB_UPD_RATE,
+				    "rate");
+}
+
 int nfp_app_set_vf_spoofchk(struct net_device *netdev, int vf, bool enable)
 {
 	struct nfp_app *app = nfp_app_from_netdev(netdev);
@@ -228,9 +262,8 @@  int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 			  struct ifla_vf_info *ivi)
 {
 	struct nfp_app *app = nfp_app_from_netdev(netdev);
-	unsigned int vf_offset;
+	u32 vf_offset, mac_hi, rate;
 	u32 vlan_tag;
-	u32 mac_hi;
 	u16 mac_lo;
 	u8 flags;
 	int err;
@@ -261,5 +294,18 @@  int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 	ivi->trusted = FIELD_GET(NFP_NET_VF_CFG_CTRL_TRUST, flags);
 	ivi->linkstate = FIELD_GET(NFP_NET_VF_CFG_CTRL_LINK_STATE, flags);
 
+	err = nfp_net_sriov_check(app, vf, NFP_NET_VF_CFG_MB_CAP_RATE, "rate");
+	if (!err) {
+		rate = readl(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_RATE);
+
+		ivi->max_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MAX_RATE, rate);
+		ivi->min_tx_rate = FIELD_GET(NFP_NET_VF_CFG_MIN_RATE, rate);
+
+		if (ivi->max_tx_rate == NFP_NET_VF_RATE_MAX)
+			ivi->max_tx_rate = 0;
+		if (ivi->min_tx_rate == NFP_NET_VF_RATE_MAX)
+			ivi->max_tx_rate = 0;
+	}
+
 	return 0;
 }
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
index 7b72cc083476..2d445fa199dc 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
@@ -20,6 +20,7 @@ 
 #define   NFP_NET_VF_CFG_MB_CAP_LINK_STATE		  (0x1 << 3)
 #define   NFP_NET_VF_CFG_MB_CAP_TRUST			  (0x1 << 4)
 #define   NFP_NET_VF_CFG_MB_CAP_VLAN_PROTO		  (0x1 << 5)
+#define   NFP_NET_VF_CFG_MB_CAP_RATE			  (0x1 << 6)
 #define NFP_NET_VF_CFG_MB_RET				0x2
 #define NFP_NET_VF_CFG_MB_UPD				0x4
 #define   NFP_NET_VF_CFG_MB_UPD_MAC			  (0x1 << 0)
@@ -28,6 +29,7 @@ 
 #define   NFP_NET_VF_CFG_MB_UPD_LINK_STATE		  (0x1 << 3)
 #define   NFP_NET_VF_CFG_MB_UPD_TRUST			  (0x1 << 4)
 #define   NFP_NET_VF_CFG_MB_UPD_VLAN_PROTO		  (0x1 << 5)
+#define   NFP_NET_VF_CFG_MB_UPD_RATE			  (0x1 << 6)
 #define NFP_NET_VF_CFG_MB_VF_NUM			0x7
 
 /* VF config entry
@@ -48,10 +50,17 @@ 
 #define   NFP_NET_VF_CFG_VLAN_PROT			  0xffff0000
 #define   NFP_NET_VF_CFG_VLAN_QOS			  0xe000
 #define   NFP_NET_VF_CFG_VLAN_VID			  0x0fff
+#define NFP_NET_VF_CFG_RATE				0xc
+#define   NFP_NET_VF_CFG_MIN_RATE			0x0000ffff
+#define   NFP_NET_VF_CFG_MAX_RATE			0xffff0000
+
+#define NFP_NET_VF_RATE_MAX			0xffff
 
 int nfp_app_set_vf_mac(struct net_device *netdev, int vf, u8 *mac);
 int nfp_app_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, u8 qos,
 			__be16 vlan_proto);
+int nfp_app_set_vf_rate(struct net_device *netdev, int vf, int min_tx_rate,
+			int max_tx_rate);
 int nfp_app_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting);
 int nfp_app_set_vf_trust(struct net_device *netdev, int vf, bool setting);
 int nfp_app_set_vf_link_state(struct net_device *netdev, int vf,