The apc->eth_stats.rx_cqes is one per NIC (vport), and it's on the
frequent and parallel code path of all queues. So, r/w into this
single shared variable by many threads on different CPUs creates a
lot caching and memory overhead, hence perf regression. And, it's
not accurate due to the high volume concurrent r/w.
Since the error path of mana_poll_rx_cq() already has warnings, so
keeping the counter and convert it to a per-queue variable is not
necessary. So, just remove this counter from this high frequency
code path.
Also, remove the tx_cqes counter for the same reason. We have
warnings & other counters for errors on that path, and don't need
to count every normal cqe processing.
Cc: [email protected]
Fixes: bd7fc6e1957c ("net: mana: Add new MANA VF performance counters for easier troubleshooting")
Signed-off-by: Haiyang Zhang <[email protected]>
---
drivers/net/ethernet/microsoft/mana/mana_en.c | 10 ----------
drivers/net/ethernet/microsoft/mana/mana_ethtool.c | 2 --
include/net/mana/mana.h | 2 --
3 files changed, 14 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 06d6292e09b3..d907727c7b7a 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1279,8 +1279,6 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
if (comp_read < 1)
return;
- apc->eth_stats.tx_cqes = comp_read;
-
for (i = 0; i < comp_read; i++) {
struct mana_tx_comp_oob *cqe_oob;
@@ -1363,8 +1361,6 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
WARN_ON_ONCE(1);
cq->work_done = pkt_transmitted;
-
- apc->eth_stats.tx_cqes -= pkt_transmitted;
}
static void mana_post_pkt_rxq(struct mana_rxq *rxq)
@@ -1626,15 +1622,11 @@ static void mana_poll_rx_cq(struct mana_cq *cq)
{
struct gdma_comp *comp = cq->gdma_comp_buf;
struct mana_rxq *rxq = cq->rxq;
- struct mana_port_context *apc;
int comp_read, i;
- apc = netdev_priv(rxq->ndev);
-
comp_read = mana_gd_poll_cq(cq->gdma_cq, comp, CQE_POLLING_BUFFER);
WARN_ON_ONCE(comp_read > CQE_POLLING_BUFFER);
- apc->eth_stats.rx_cqes = comp_read;
rxq->xdp_flush = false;
for (i = 0; i < comp_read; i++) {
@@ -1646,8 +1638,6 @@ static void mana_poll_rx_cq(struct mana_cq *cq)
return;
mana_process_rx_cqe(rxq, cq, &comp[i]);
-
- apc->eth_stats.rx_cqes--;
}
if (rxq->xdp_flush)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index a64c81410dc1..0dc78679f620 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -13,11 +13,9 @@ static const struct {
} mana_eth_stats[] = {
{"stop_queue", offsetof(struct mana_ethtool_stats, stop_queue)},
{"wake_queue", offsetof(struct mana_ethtool_stats, wake_queue)},
- {"tx_cqes", offsetof(struct mana_ethtool_stats, tx_cqes)},
{"tx_cq_err", offsetof(struct mana_ethtool_stats, tx_cqe_err)},
{"tx_cqe_unknown_type", offsetof(struct mana_ethtool_stats,
tx_cqe_unknown_type)},
- {"rx_cqes", offsetof(struct mana_ethtool_stats, rx_cqes)},
{"rx_coalesced_err", offsetof(struct mana_ethtool_stats,
rx_coalesced_err)},
{"rx_cqe_unknown_type", offsetof(struct mana_ethtool_stats,
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index cd386aa7c7cc..9eef19972845 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -347,10 +347,8 @@ struct mana_tx_qp {
struct mana_ethtool_stats {
u64 stop_queue;
u64 wake_queue;
- u64 tx_cqes;
u64 tx_cqe_err;
u64 tx_cqe_unknown_type;
- u64 rx_cqes;
u64 rx_coalesced_err;
u64 rx_cqe_unknown_type;
};
--
2.25.1
The 05/24/2023 14:22, Haiyang Zhang wrote:
Hi Haiyang,
>
> The apc->eth_stats.rx_cqes is one per NIC (vport), and it's on the
> frequent and parallel code path of all queues. So, r/w into this
> single shared variable by many threads on different CPUs creates a
> lot caching and memory overhead, hence perf regression. And, it's
> not accurate due to the high volume concurrent r/w.
Do you have any numbers to show the improvement of this change?
>
> Since the error path of mana_poll_rx_cq() already has warnings, so
> keeping the counter and convert it to a per-queue variable is not
> necessary. So, just remove this counter from this high frequency
> code path.
>
> Also, remove the tx_cqes counter for the same reason. We have
> warnings & other counters for errors on that path, and don't need
> to count every normal cqe processing.
Will you not have problems with the counter 'apc->eth_stats.tx_cqe_err'?
It is not in the hot path but you will have concurrent access to it.
>
> Cc: [email protected]
> Fixes: bd7fc6e1957c ("net: mana: Add new MANA VF performance counters for easier troubleshooting")
> Signed-off-by: Haiyang Zhang <[email protected]>
> ---
> drivers/net/ethernet/microsoft/mana/mana_en.c | 10 ----------
> drivers/net/ethernet/microsoft/mana/mana_ethtool.c | 2 --
> include/net/mana/mana.h | 2 --
> 3 files changed, 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 06d6292e09b3..d907727c7b7a 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1279,8 +1279,6 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
> if (comp_read < 1)
> return;
>
> - apc->eth_stats.tx_cqes = comp_read;
> -
> for (i = 0; i < comp_read; i++) {
> struct mana_tx_comp_oob *cqe_oob;
>
> @@ -1363,8 +1361,6 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
> WARN_ON_ONCE(1);
>
> cq->work_done = pkt_transmitted;
> -
> - apc->eth_stats.tx_cqes -= pkt_transmitted;
> }
>
> static void mana_post_pkt_rxq(struct mana_rxq *rxq)
> @@ -1626,15 +1622,11 @@ static void mana_poll_rx_cq(struct mana_cq *cq)
> {
> struct gdma_comp *comp = cq->gdma_comp_buf;
> struct mana_rxq *rxq = cq->rxq;
> - struct mana_port_context *apc;
> int comp_read, i;
>
> - apc = netdev_priv(rxq->ndev);
> -
> comp_read = mana_gd_poll_cq(cq->gdma_cq, comp, CQE_POLLING_BUFFER);
> WARN_ON_ONCE(comp_read > CQE_POLLING_BUFFER);
>
> - apc->eth_stats.rx_cqes = comp_read;
> rxq->xdp_flush = false;
>
> for (i = 0; i < comp_read; i++) {
> @@ -1646,8 +1638,6 @@ static void mana_poll_rx_cq(struct mana_cq *cq)
> return;
>
> mana_process_rx_cqe(rxq, cq, &comp[i]);
> -
> - apc->eth_stats.rx_cqes--;
> }
>
> if (rxq->xdp_flush)
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index a64c81410dc1..0dc78679f620 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -13,11 +13,9 @@ static const struct {
> } mana_eth_stats[] = {
> {"stop_queue", offsetof(struct mana_ethtool_stats, stop_queue)},
> {"wake_queue", offsetof(struct mana_ethtool_stats, wake_queue)},
> - {"tx_cqes", offsetof(struct mana_ethtool_stats, tx_cqes)},
> {"tx_cq_err", offsetof(struct mana_ethtool_stats, tx_cqe_err)},
> {"tx_cqe_unknown_type", offsetof(struct mana_ethtool_stats,
> tx_cqe_unknown_type)},
> - {"rx_cqes", offsetof(struct mana_ethtool_stats, rx_cqes)},
> {"rx_coalesced_err", offsetof(struct mana_ethtool_stats,
> rx_coalesced_err)},
> {"rx_cqe_unknown_type", offsetof(struct mana_ethtool_stats,
> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index cd386aa7c7cc..9eef19972845 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -347,10 +347,8 @@ struct mana_tx_qp {
> struct mana_ethtool_stats {
> u64 stop_queue;
> u64 wake_queue;
> - u64 tx_cqes;
> u64 tx_cqe_err;
> u64 tx_cqe_unknown_type;
> - u64 rx_cqes;
> u64 rx_coalesced_err;
> u64 rx_cqe_unknown_type;
> };
> --
> 2.25.1
>
>
--
/Horatiu
> -----Original Message-----
> From: Horatiu Vultur <[email protected]>
> Sent: Thursday, May 25, 2023 2:49 AM
> To: Haiyang Zhang <[email protected]>
> Cc: [email protected]; [email protected]; Dexuan Cui
> <[email protected]>; KY Srinivasan <[email protected]>; Paul Rosswurm
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Long Li
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Ajay Sharma
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH net] net: mana: Fix perf regression: remove rx_cqes,
> tx_cqes counters
>
> [Some people who received this message don't often get email from
> [email protected]. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>
> The 05/24/2023 14:22, Haiyang Zhang wrote:
>
> Hi Haiyang,
>
> >
> > The apc->eth_stats.rx_cqes is one per NIC (vport), and it's on the
> > frequent and parallel code path of all queues. So, r/w into this
> > single shared variable by many threads on different CPUs creates a
> > lot caching and memory overhead, hence perf regression. And, it's
> > not accurate due to the high volume concurrent r/w.
>
> Do you have any numbers to show the improvement of this change?
The numbers are not published. The perf regression of the previous
patch is very significant, and this patch eliminates the regression.
>
> >
> > Since the error path of mana_poll_rx_cq() already has warnings, so
> > keeping the counter and convert it to a per-queue variable is not
> > necessary. So, just remove this counter from this high frequency
> > code path.
> >
> > Also, remove the tx_cqes counter for the same reason. We have
> > warnings & other counters for errors on that path, and don't need
> > to count every normal cqe processing.
>
> Will you not have problems with the counter 'apc->eth_stats.tx_cqe_err'?
> It is not in the hot path but you will have concurrent access to it.
Yes, but that error happens rarely, so a shared variable is good enough. So, I
don't change it in this patch.
Thanks,
- Haiyang
The 05/25/2023 14:34, Haiyang Zhang wrote:
>
> > -----Original Message-----
> > From: Horatiu Vultur <[email protected]>
> > Sent: Thursday, May 25, 2023 2:49 AM
> > To: Haiyang Zhang <[email protected]>
> > Cc: [email protected]; [email protected]; Dexuan Cui
> > <[email protected]>; KY Srinivasan <[email protected]>; Paul Rosswurm
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; Long Li
> > <[email protected]>; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Ajay Sharma
> > <[email protected]>; [email protected]; linux-
> > [email protected]; [email protected]
> > Subject: Re: [PATCH net] net: mana: Fix perf regression: remove rx_cqes,
> > tx_cqes counters
> >
> > [Some people who received this message don't often get email from
> > [email protected]. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> >
> > The 05/24/2023 14:22, Haiyang Zhang wrote:
> >
> > Hi Haiyang,
> >
> > >
> > > The apc->eth_stats.rx_cqes is one per NIC (vport), and it's on the
> > > frequent and parallel code path of all queues. So, r/w into this
> > > single shared variable by many threads on different CPUs creates a
> > > lot caching and memory overhead, hence perf regression. And, it's
> > > not accurate due to the high volume concurrent r/w.
> >
> > Do you have any numbers to show the improvement of this change?
>
> The numbers are not published. The perf regression of the previous
> patch is very significant, and this patch eliminates the regression.
>
> >
> > >
> > > Since the error path of mana_poll_rx_cq() already has warnings, so
> > > keeping the counter and convert it to a per-queue variable is not
> > > necessary. So, just remove this counter from this high frequency
> > > code path.
> > >
> > > Also, remove the tx_cqes counter for the same reason. We have
> > > warnings & other counters for errors on that path, and don't need
> > > to count every normal cqe processing.
> >
> > Will you not have problems with the counter 'apc->eth_stats.tx_cqe_err'?
> > It is not in the hot path but you will have concurrent access to it.
>
> Yes, but that error happens rarely, so a shared variable is good enough. So, I
> don't change it in this patch.
OK, I understand.
Maybe this can be fixed in a different patch at a later point. Thanks.
Reviwed-by: Horatiu Vultur <[email protected]>
>
> Thanks,
> - Haiyang
>
--
/Horatiu
Wed, May 24, 2023 at 11:22:00PM CEST, [email protected] wrote:
>The apc->eth_stats.rx_cqes is one per NIC (vport), and it's on the
>frequent and parallel code path of all queues. So, r/w into this
>single shared variable by many threads on different CPUs creates a
>lot caching and memory overhead, hence perf regression. And, it's
>not accurate due to the high volume concurrent r/w.
>
>Since the error path of mana_poll_rx_cq() already has warnings, so
>keeping the counter and convert it to a per-queue variable is not
>necessary. So, just remove this counter from this high frequency
>code path.
>
>Also, remove the tx_cqes counter for the same reason. We have
>warnings & other counters for errors on that path, and don't need
>to count every normal cqe processing.
>
>Cc: [email protected]
>Fixes: bd7fc6e1957c ("net: mana: Add new MANA VF performance counters for easier troubleshooting")
>Signed-off-by: Haiyang Zhang <[email protected]>
Reviewed-by: Jiri Pirko <[email protected]>