[PATCH net-next 0/2] nfp: series of minor driver improvements

[PATCH net-next 0/2] nfp: series of minor driver improvements

[Louis Peens]

This short series bundles two unrelated but small updates to the nfp
driver.

Patch1: Adds a new devlink info field to expose an upcoming hardware
        information field.
Patch2: Customize the dim profiles for coalescing on the nfp.

Fei Qin (2):
  nfp: update devlink device info output
  nfp: customize the dim profiles

 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  1 +
 .../ethernet/netronome/nfp/nfp_net_common.c   | 27 ++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH net-next 1/2] nfp: update devlink device info output

[Louis Peens]

From: Fei Qin <fei.qin@corigine.com>

Newer NIC will introduce a new part number field, add it to devlink
device info.

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 635d33c0d6d3..91563b705639 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -160,6 +160,7 @@ static const struct nfp_devlink_versions_simple {
 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
 	{ "board.model", /* code name */		"assembly.model", },
+	{ "board.pn",					"pn", },
 };
 
 static int
-- 
2.34.1

[PATCH net-next 2/2] nfp: customize the dim profiles

[Louis Peens]

From: Fei Qin <fei.qin@corigine.com>

The latency with default profiles is not very good when adaptive
interrupt moderation is enabled. This patch customizes the dim
profiles to optimize the latency.

Latency comparison between default and customized profiles for 5
different runs:
                                     Latency (us)
Default profiles     |   158.79 158.05 158.46 157.93 157.42
Customized profiles  |   107.03 106.46 113.01 131.64 107.94

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 .../ethernet/netronome/nfp/nfp_net_common.c   | 27 ++++++++++++++++---
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 3b3210d823e8..cfbcec3045bf 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -1158,16 +1158,28 @@ void nfp_ctrl_close(struct nfp_net *nn)
 	rtnl_unlock();
 }
 
+struct nfp_dim {
+	u16 usec;
+	u16 pkts;
+};
+
 static void nfp_net_rx_dim_work(struct work_struct *work)
 {
+	static const struct nfp_dim rx_profile[] = {
+		{.usec = 0, .pkts = 1},
+		{.usec = 4, .pkts = 32},
+		{.usec = 64, .pkts = 64},
+		{.usec = 128, .pkts = 256},
+		{.usec = 256, .pkts = 256},
+	};
 	struct nfp_net_r_vector *r_vec;
 	unsigned int factor, value;
-	struct dim_cq_moder moder;
+	struct nfp_dim moder;
 	struct nfp_net *nn;
 	struct dim *dim;
 
 	dim = container_of(work, struct dim, work);
-	moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
+	moder = rx_profile[dim->profile_ix];
 	r_vec = container_of(dim, struct nfp_net_r_vector, rx_dim);
 	nn = r_vec->nfp_net;
 
@@ -1190,14 +1202,21 @@ static void nfp_net_rx_dim_work(struct work_struct *work)
 
 static void nfp_net_tx_dim_work(struct work_struct *work)
 {
+	static const struct nfp_dim tx_profile[] = {
+		{.usec = 0, .pkts = 1},
+		{.usec = 4, .pkts = 16},
+		{.usec = 32, .pkts = 64},
+		{.usec = 64, .pkts = 128},
+		{.usec = 128, .pkts = 128},
+	};
 	struct nfp_net_r_vector *r_vec;
 	unsigned int factor, value;
-	struct dim_cq_moder moder;
+	struct nfp_dim moder;
 	struct nfp_net *nn;
 	struct dim *dim;
 
 	dim = container_of(work, struct dim, work);
-	moder = net_dim_get_tx_moderation(dim->mode, dim->profile_ix);
+	moder = tx_profile[dim->profile_ix];
 	r_vec = container_of(dim, struct nfp_net_r_vector, tx_dim);
 	nn = r_vec->nfp_net;
 
-- 
2.34.1

Re: [PATCH net-next 1/2] nfp: update devlink device info output

[Jiri Pirko]

Wed, Jan 31, 2024 at 09:54:25AM CET, louis.peens@corigine.com wrote:
>From: Fei Qin <fei.qin@corigine.com>
>
>Newer NIC will introduce a new part number field, add it to devlink
>device info.
>
>Signed-off-by: Fei Qin <fei.qin@corigine.com>
>Signed-off-by: Louis Peens <louis.peens@corigine.com>
>---
> drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index 635d33c0d6d3..91563b705639 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -160,6 +160,7 @@ static const struct nfp_devlink_versions_simple {
> 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
> 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
> 	{ "board.model", /* code name */		"assembly.model", },
>+	{ "board.pn",					"pn", },

This looks quite generic. Could you please introduce:
DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL
DEVLINK_INFO_VERSION_GENERIC_BOARD_PN
and use those while you are at it?

Thanks!

pw-bot: cr

Re: [PATCH net-next 2/2] nfp: customize the dim profiles

[Jiri Pirko]

Wed, Jan 31, 2024 at 09:54:26AM CET, louis.peens@corigine.com wrote:
>From: Fei Qin <fei.qin@corigine.com>
>
>The latency with default profiles is not very good when adaptive
>interrupt moderation is enabled. This patch customizes the dim
>profiles to optimize the latency.
>
>Latency comparison between default and customized profiles for 5
>different runs:
>                                     Latency (us)
>Default profiles     |   158.79 158.05 158.46 157.93 157.42
>Customized profiles  |   107.03 106.46 113.01 131.64 107.94
>
>Signed-off-by: Fei Qin <fei.qin@corigine.com>
>Signed-off-by: Louis Peens <louis.peens@corigine.com>
>---
> .../ethernet/netronome/nfp/nfp_net_common.c   | 27 ++++++++++++++++---
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
>index 3b3210d823e8..cfbcec3045bf 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
>@@ -1158,16 +1158,28 @@ void nfp_ctrl_close(struct nfp_net *nn)
> 	rtnl_unlock();
> }
> 
>+struct nfp_dim {
>+	u16 usec;
>+	u16 pkts;
>+};
>+
> static void nfp_net_rx_dim_work(struct work_struct *work)
> {
>+	static const struct nfp_dim rx_profile[] = {
>+		{.usec = 0, .pkts = 1},
>+		{.usec = 4, .pkts = 32},
>+		{.usec = 64, .pkts = 64},
>+		{.usec = 128, .pkts = 256},
>+		{.usec = 256, .pkts = 256},
>+	};
> 	struct nfp_net_r_vector *r_vec;
> 	unsigned int factor, value;
>-	struct dim_cq_moder moder;
>+	struct nfp_dim moder;
> 	struct nfp_net *nn;
> 	struct dim *dim;
> 
> 	dim = container_of(work, struct dim, work);
>-	moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
>+	moder = rx_profile[dim->profile_ix];

It looks incorrect to hardcode it like this. There is a reason this is
abstracted out in lib/dim/net_dim.c to avoid exactly this. Can't you
perhaps introduce your modified profile there and keep using
net_dim_get_[tr]x_moderation() helpers?



> 	r_vec = container_of(dim, struct nfp_net_r_vector, rx_dim);
> 	nn = r_vec->nfp_net;
> 
>@@ -1190,14 +1202,21 @@ static void nfp_net_rx_dim_work(struct work_struct *work)
> 
> static void nfp_net_tx_dim_work(struct work_struct *work)
> {
>+	static const struct nfp_dim tx_profile[] = {
>+		{.usec = 0, .pkts = 1},
>+		{.usec = 4, .pkts = 16},
>+		{.usec = 32, .pkts = 64},
>+		{.usec = 64, .pkts = 128},
>+		{.usec = 128, .pkts = 128},
>+	};
> 	struct nfp_net_r_vector *r_vec;
> 	unsigned int factor, value;
>-	struct dim_cq_moder moder;
>+	struct nfp_dim moder;
> 	struct nfp_net *nn;
> 	struct dim *dim;
> 
> 	dim = container_of(work, struct dim, work);
>-	moder = net_dim_get_tx_moderation(dim->mode, dim->profile_ix);
>+	moder = tx_profile[dim->profile_ix];
> 	r_vec = container_of(dim, struct nfp_net_r_vector, tx_dim);
> 	nn = r_vec->nfp_net;
> 
>-- 
>2.34.1
>
>

Re: [PATCH net-next 1/2] nfp: update devlink device info output

[Louis Peens]

On Wed, Jan 31, 2024 at 10:31:51AM +0100, Jiri Pirko wrote:
> Wed, Jan 31, 2024 at 09:54:25AM CET, louis.peens@corigine.com wrote:
> >From: Fei Qin <fei.qin@corigine.com>
> >
> >Newer NIC will introduce a new part number field, add it to devlink
> >device info.
> >
> >Signed-off-by: Fei Qin <fei.qin@corigine.com>
> >Signed-off-by: Louis Peens <louis.peens@corigine.com>
> >---
> > drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >index 635d33c0d6d3..91563b705639 100644
> >--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >@@ -160,6 +160,7 @@ static const struct nfp_devlink_versions_simple {
> > 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
> > 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
> > 	{ "board.model", /* code name */		"assembly.model", },
> >+	{ "board.pn",					"pn", },
> 
> This looks quite generic. Could you please introduce:
> DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL
> DEVLINK_INFO_VERSION_GENERIC_BOARD_PN
> and use those while you are at it?
We will do so, thanks.
> 
> Thanks!
> 
> pw-bot: cr

Re: [PATCH net-next 2/2] nfp: customize the dim profiles

[Louis Peens]

On Wed, Jan 31, 2024 at 10:39:01AM +0100, Jiri Pirko wrote:
> [Some people who received this message don't often get email from jiri@resnulli.us. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> > static void nfp_net_rx_dim_work(struct work_struct *work)
> > {
> >+      static const struct nfp_dim rx_profile[] = {
> >+              {.usec = 0, .pkts = 1},
> >+              {.usec = 4, .pkts = 32},
> >+              {.usec = 64, .pkts = 64},
> >+              {.usec = 128, .pkts = 256},
> >+              {.usec = 256, .pkts = 256},
> >+      };
> >       struct nfp_net_r_vector *r_vec;
> >       unsigned int factor, value;
> >-      struct dim_cq_moder moder;
> >+      struct nfp_dim moder;
> >       struct nfp_net *nn;
> >       struct dim *dim;
> >
> >       dim = container_of(work, struct dim, work);
> >-      moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
> >+      moder = rx_profile[dim->profile_ix];
> 
> It looks incorrect to hardcode it like this. There is a reason this is
> abstracted out in lib/dim/net_dim.c to avoid exactly this. Can't you
> perhaps introduce your modified profile there and keep using
> net_dim_get_[tr]x_moderation() helpers?

Hmmm, we'll take a look at this. There might be follow-up questions but
we'll give it a closer look first. Thanks for the feedback.

RE: [PATCH net-next 2/2] nfp: customize the dim profiles

[Yinjun Zhang]

On Wednesday, January 31, 2024 5:39 PM, Jiri Pirko wrote:
<...>
> It looks incorrect to hardcode it like this. There is a reason this is
> abstracted out in lib/dim/net_dim.c to avoid exactly this. Can't you
> perhaps introduce your modified profile there and keep using
> net_dim_get_[tr]x_moderation() helpers?
> 

We don't know if this introduced profile is adaptable to other NICs/vendors,
it's generated based on NFP's performance. Do you really think it's
appropriate to move it to the net_dim.c as a new common profile like:
enum dim_cq_period_mode {
        DIM_CQ_PERIOD_MODE_START_FROM_EQE = 0x0,
        DIM_CQ_PERIOD_MODE_START_FROM_CQE = 0x1,
+       DIM_CQ_PERIOD_MODE_SPECIFIC_0 = 0x2,
        DIM_CQ_PERIOD_NUM_MODES
 };

Re: [PATCH net-next 2/2] nfp: customize the dim profiles

[Jiri Pirko]

Thu, Feb 01, 2024 at 03:16:50AM CET, yinjun.zhang@corigine.com wrote:
>On Wednesday, January 31, 2024 5:39 PM, Jiri Pirko wrote:
><...>
>> It looks incorrect to hardcode it like this. There is a reason this is
>> abstracted out in lib/dim/net_dim.c to avoid exactly this. Can't you
>> perhaps introduce your modified profile there and keep using
>> net_dim_get_[tr]x_moderation() helpers?
>> 
>
>We don't know if this introduced profile is adaptable to other NICs/vendors,
>it's generated based on NFP's performance. Do you really think it's
>appropriate to move it to the net_dim.c as a new common profile like:
>enum dim_cq_period_mode {
>        DIM_CQ_PERIOD_MODE_START_FROM_EQE = 0x0,
>        DIM_CQ_PERIOD_MODE_START_FROM_CQE = 0x1,
>+       DIM_CQ_PERIOD_MODE_SPECIFIC_0 = 0x2,

Maybe. Can't think of anything better atm. Maybe others would have some
ideas.

>        DIM_CQ_PERIOD_NUM_MODES
> };
>
>

Re: [PATCH net-next 1/2] nfp: update devlink device info output

[Jakub Kicinski]

On Wed, 31 Jan 2024 10:54:25 +0200 Louis Peens wrote:
> +	{ "board.pn",					"pn", },

"part_number", spell it out, please.
Why "board"? Part number is for the entire product, isn't it?