net/ixgbe: fix qos sched sample app performance drop
Checks
Commit Message
Currently MACsec register is set without any conditions when start port.
It should be set only when user needs. To avoid wild value, I add init
function. This patch fixes the issue.
Fixes: 50556c88104c ("net/ixgbe: fix MACsec setting")
Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
Comments
Hi,
Guinan, could you take a look at this patch as well.
On 11/20, Shougang Wang wrote:
>Currently MACsec register is set without any conditions when start port.
>It should be set only when user needs. To avoid wild value, I add init
>function. This patch fixes the issue.
>
>Fixes: 50556c88104c ("net/ixgbe: fix MACsec setting")
>
>Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>index 8c1caac18..d54eec1c6 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -379,6 +379,8 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
> static int ixgbe_filter_restore(struct rte_eth_dev *dev);
> static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
>
>+static void ixgbe_dev_macsec_init(struct rte_eth_dev *dev);
>+
> /*
> * Define VF Stats MACRO for Non "cleared on read" register
> */
>@@ -1095,6 +1097,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
>
> PMD_INIT_FUNC_TRACE();
>
>+ ixgbe_dev_macsec_init(eth_dev);
This is not needed as dev_private is allocated by rte_zmalloc_socket.
>+
> eth_dev->dev_ops = &ixgbe_eth_dev_ops;
> eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
> eth_dev->tx_pkt_burst = &ixgbe_xmit_pkts;
>@@ -2545,7 +2549,7 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> uint32_t *link_speeds;
> struct ixgbe_tm_conf *tm_conf =
> IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
>- struct ixgbe_macsec_setting *macsec_ctrl =
>+ struct ixgbe_macsec_setting *macsec_setting =
> IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
>
> PMD_INIT_FUNC_TRACE();
>@@ -2799,9 +2803,11 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> */
> ixgbe_dev_link_update(dev, 0);
>
>- /* setup the macsec ctrl register */
>- ixgbe_dev_macsec_register_enable(dev, macsec_ctrl);
>-
>+ /* setup the macsec setting register */
>+ if (macsec_setting->encrypt_en != 0 ||
>+ macsec_setting->replayprotect_en != 0) {
>+ ixgbe_dev_macsec_register_enable(dev, macsec_setting);
>+ }
Can we safely assume that if encrypt_en and replayprotect_en equals zero, then
we don't need to call ixgbe_dev_macsec_register_enable at all? Since this enable
routine is more about just encrypt_en/replayprotect_en, is that any usercase
when user set macsec offload with both encrypt_en and replayprotect_en are 0?
Thanks,
Xiaolong
> return 0;
>
> error:
>@@ -2827,6 +2833,8 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> int vf;
> struct ixgbe_tm_conf *tm_conf =
> IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
>+ struct ixgbe_macsec_setting *macsec_setting =
>+ IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
>
> if (hw->adapter_stopped)
> return;
>@@ -2834,7 +2842,10 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> PMD_INIT_FUNC_TRACE();
>
> /* disable mecsec register */
>- ixgbe_dev_macsec_register_disable(dev);
>+ if (macsec_setting->encrypt_en != 0 ||
>+ macsec_setting->replayprotect_en != 0) {
>+ ixgbe_dev_macsec_register_disable(dev);
>+ }
>
> rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
>
>@@ -8977,6 +8988,16 @@ ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev)
> ixgbe_enable_sec_tx_path_generic(hw);
> }
>
>+static void
>+ixgbe_dev_macsec_init(struct rte_eth_dev *dev)
>+{
>+ struct ixgbe_macsec_setting *macsec =
>+ IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
>+
>+ macsec->encrypt_en = 0;
>+ macsec->replayprotect_en = 0;
>+}
>+
> RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
> RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
>--
>2.17.1
>
Hi, Xiaolong
> -----Original Message-----
[snip]
> >+static void ixgbe_dev_macsec_init(struct rte_eth_dev *dev);
> >+
> > /*
> > * Define VF Stats MACRO for Non "cleared on read" register
> > */
> >@@ -1095,6 +1097,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev,
> >void *init_params __rte_unused)
> >
> > PMD_INIT_FUNC_TRACE();
> >
> >+ ixgbe_dev_macsec_init(eth_dev);
>
> This is not needed as dev_private is allocated by rte_zmalloc_socket.
>
OK, I will remove it.
> >+
> > eth_dev->dev_ops = &ixgbe_eth_dev_ops;
> > eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
> > eth_dev->tx_pkt_burst = &ixgbe_xmit_pkts; @@ -2545,7 +2549,7 @@
> >ixgbe_dev_start(struct rte_eth_dev *dev)
> > uint32_t *link_speeds;
> > struct ixgbe_tm_conf *tm_conf =
> > IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
> >- struct ixgbe_macsec_setting *macsec_ctrl =
> >+ struct ixgbe_macsec_setting *macsec_setting =
> > IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data-
> >dev_private);
> >
> > PMD_INIT_FUNC_TRACE();
> >@@ -2799,9 +2803,11 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> > */
> > ixgbe_dev_link_update(dev, 0);
> >
> >- /* setup the macsec ctrl register */
> >- ixgbe_dev_macsec_register_enable(dev, macsec_ctrl);
> >-
> >+ /* setup the macsec setting register */
> >+ if (macsec_setting->encrypt_en != 0 ||
> >+ macsec_setting->replayprotect_en != 0) {
> >+ ixgbe_dev_macsec_register_enable(dev, macsec_setting);
> >+ }
>
> Can we safely assume that if encrypt_en and replayprotect_en equals zero,
> then we don't need to call ixgbe_dev_macsec_register_enable at all? Since this
> enable routine is more about just encrypt_en/replayprotect_en, is that any
> usercase when user set macsec offload with both encrypt_en and
> replayprotect_en are 0?
>
As you said, this is a problem. I will fix it. Thanks a lot.
Thanks.
Shougang
@@ -379,6 +379,8 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
static int ixgbe_filter_restore(struct rte_eth_dev *dev);
static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
+static void ixgbe_dev_macsec_init(struct rte_eth_dev *dev);
+
/*
* Define VF Stats MACRO for Non "cleared on read" register
*/
@@ -1095,6 +1097,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
PMD_INIT_FUNC_TRACE();
+ ixgbe_dev_macsec_init(eth_dev);
+
eth_dev->dev_ops = &ixgbe_eth_dev_ops;
eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
eth_dev->tx_pkt_burst = &ixgbe_xmit_pkts;
@@ -2545,7 +2549,7 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
uint32_t *link_speeds;
struct ixgbe_tm_conf *tm_conf =
IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
- struct ixgbe_macsec_setting *macsec_ctrl =
+ struct ixgbe_macsec_setting *macsec_setting =
IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
PMD_INIT_FUNC_TRACE();
@@ -2799,9 +2803,11 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
*/
ixgbe_dev_link_update(dev, 0);
- /* setup the macsec ctrl register */
- ixgbe_dev_macsec_register_enable(dev, macsec_ctrl);
-
+ /* setup the macsec setting register */
+ if (macsec_setting->encrypt_en != 0 ||
+ macsec_setting->replayprotect_en != 0) {
+ ixgbe_dev_macsec_register_enable(dev, macsec_setting);
+ }
return 0;
error:
@@ -2827,6 +2833,8 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
int vf;
struct ixgbe_tm_conf *tm_conf =
IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+ struct ixgbe_macsec_setting *macsec_setting =
+ IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
if (hw->adapter_stopped)
return;
@@ -2834,7 +2842,10 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
PMD_INIT_FUNC_TRACE();
/* disable mecsec register */
- ixgbe_dev_macsec_register_disable(dev);
+ if (macsec_setting->encrypt_en != 0 ||
+ macsec_setting->replayprotect_en != 0) {
+ ixgbe_dev_macsec_register_disable(dev);
+ }
rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
@@ -8977,6 +8988,16 @@ ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev)
ixgbe_enable_sec_tx_path_generic(hw);
}
+static void
+ixgbe_dev_macsec_init(struct rte_eth_dev *dev)
+{
+ struct ixgbe_macsec_setting *macsec =
+ IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
+
+ macsec->encrypt_en = 0;
+ macsec->replayprotect_en = 0;
+}
+
RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");