rte_bus_probe() does not exit on error

rte_bus_probe() does not exit on error

[McDaniel, Timothy]

Hello DPDK community,

I am following up on a question/comment that I submitted on April 18, for which
I have not received any responses. See the original comment below for context.

Are there objections to modifying the behavior of rte_bus_probe() so that it propagates
any errors detected while processing the command line arguments? It currently ignores
errors and continues on, always returning success instead of any error that was returned
by the probe function.

Thank You,
Tim McDaniel

> -----Original Message-----
> From: dev-request@dpdk.org <dev-request@dpdk.org>
> Sent: Monday, April 18, 2022 8:20 AM
> To: dev@dpdk.org
> Subject: dev Digest, Vol 400, Issue 6
> 
> Send dev mailing list submissions to
> 	dev@dpdk.org
> 
> To subscribe or unsubscribe via the World Wide Web, visit
> 	https://mails.dpdk.org/listinfo/dev
> or, via email, send a message with subject or body 'help' to
> 	dev-request@dpdk.org
> 
> You can reach the person managing the list at
> 	dev-owner@dpdk.org
> 
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of dev digest..."
> 
> 
> Today's Topics:
> 
>    1. [PATCH 1/2] common/mlx5: extend crypto capabilities (Raja Zidane)
>    2. RE: [PATCH] kni: update kernel API to receive packets
>       (Gagandeep Singh)
>    3. rte_bus_probe() does not exit on error (McDaniel, Timothy)
> 
> 
> ----------------------------------------------------------------------
> 
> Message: 1
> Date: Mon, 18 Apr 2022 14:10:04 +0300
> From: Raja Zidane <rzidane@nvidia.com>
> To: <dev@dpdk.org>
> Cc: <matan@nvidia.com>
> Subject: [PATCH 1/2] common/mlx5: extend crypto capabilities
> Message-ID: <20220418111005.2291-2-rzidane@nvidia.com>
> Content-Type: text/plain
> 
> Crypto capabilities struct contains info about crypto import method
> (wrapped/plaintext DEK) for each of the supported algorithms.
> 
> Query crypto capabilities struct and save import methods.
> 
> Signed-off-by: Raja Zidane <rzidane@nvidia.com>
> ---
>  drivers/common/mlx5/mlx5_devx_cmds.c | 13 +++++++++++--
>  drivers/common/mlx5/mlx5_devx_cmds.h |  1 +
>  drivers/common/mlx5/mlx5_prm.h       | 29 ++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c
> b/drivers/common/mlx5/mlx5_devx_cmds.c
> index d02ac2a678..c6bdbc12bb 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -964,12 +964,21 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
>  		MLX5_GET(cmd_hca_cap, hcattr,
> umr_modify_entity_size_disabled);
>  	attr->wait_on_time = MLX5_GET(cmd_hca_cap, hcattr, wait_on_time);
>  	attr->crypto = MLX5_GET(cmd_hca_cap, hcattr, crypto);
> -	if (attr->crypto)
> -		attr->aes_xts = MLX5_GET(cmd_hca_cap, hcattr, aes_xts);
>  	attr->ct_offload = !!(MLX5_GET64(cmd_hca_cap, hcattr,
>  					 general_obj_types) &
> 
> MLX5_GENERAL_OBJ_TYPES_CAP_CONN_TRACK_OFFLOAD);
>  	attr->rq_delay_drop = MLX5_GET(cmd_hca_cap, hcattr, rq_delay_drop);
> +	if (attr->crypto) {
> +		attr->aes_xts = MLX5_GET(cmd_hca_cap, hcattr, aes_xts);
> +		hcattr = mlx5_devx_get_hca_cap(ctx, in, out, &rc,
> +				MLX5_GET_HCA_CAP_OP_MOD_CRYPTO |
> +				MLX5_HCA_CAP_OPMOD_GET_CUR);
> +		if (!hcattr)
> +			return -1;
> +		attr->crypto_wrapped_import_method =
> !!(MLX5_GET(crypto_caps,
> +						hcattr,
> wrapped_import_method)
> +						& 1 << 2);
> +	}
>  	if (hca_cap_2_sup) {
>  		hcattr = mlx5_devx_get_hca_cap(ctx, in, out, &rc,
> 
> 	MLX5_GET_HCA_CAP_OP_MOD_GENERAL_DEVICE_2 |
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h
> b/drivers/common/mlx5/mlx5_devx_cmds.h
> index 1bac18c59d..3747ef9e33 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.h
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.h
> @@ -254,6 +254,7 @@ struct mlx5_hca_attr {
>  	uint32_t umr_indirect_mkey_disabled:1;
>  	uint32_t log_min_stride_wqe_sz:5;
>  	uint32_t esw_mgr_vport_id_valid:1; /* E-Switch Mgr vport ID is valid. */
> +	uint32_t crypto_wrapped_import_method:1;
>  	uint16_t esw_mgr_vport_id; /* E-Switch Mgr vport ID . */
>  	uint16_t max_wqe_sz_sq;
>  };
> diff --git a/drivers/common/mlx5/mlx5_prm.h
> b/drivers/common/mlx5/mlx5_prm.h
> index 44b18225f6..d04c9e32c9 100644
> --- a/drivers/common/mlx5/mlx5_prm.h
> +++ b/drivers/common/mlx5/mlx5_prm.h
> @@ -1293,6 +1293,7 @@ enum {
>  	MLX5_GET_HCA_CAP_OP_MOD_NIC_FLOW_TABLE = 0x7 << 1,
>  	MLX5_SET_HCA_CAP_OP_MOD_ESW = 0x9 << 1,
>  	MLX5_GET_HCA_CAP_OP_MOD_VDPA_EMULATION = 0x13 << 1,
> +	MLX5_GET_HCA_CAP_OP_MOD_CRYPTO = 0x1A << 1,
>  	MLX5_GET_HCA_CAP_OP_MOD_PARSE_GRAPH_NODE_CAP = 0x1C <<
> 1,
>  	MLX5_GET_HCA_CAP_OP_MOD_GENERAL_DEVICE_2 = 0x20 << 1,
>  };
> @@ -3794,6 +3795,34 @@ struct mlx5_ifc_crypto_operational_register_bits {
>  	u8 reserved_at_280[0x180];
>  };
> 
> +struct mlx5_ifc_crypto_caps_bits {
> +	u8 wrapped_crypto_operational[0x1];
> +	u8 wrapped_crypto_going_to_commissioning[0x1];
> +	u8 sw_wrapped_dek[0x1];
> +	u8 synchronize_dek[0x1];
> +	u8 int_kek_manual[0x1];
> +	u8 int_kek_auto[0x1];
> +	u8 reserved_at_6[0x12];
> +	u8 wrapped_import_method[0x8];
> +	u8 reserved_at_20[0x3];
> +	u8 log_dek_max_alloc[0x5];
> +	u8 reserved_at_28[0x3];
> +	u8 log_max_num_deks[0x5];
> +	u8 reserved_at_30[0x3];
> +	u8 log_max_num_import_keks[0x5];
> +	u8 reserved_at_38[0x3];
> +	u8 log_max_num_creds[0x5];
> +	u8 failed_selftests[0x10];
> +	u8 num_nv_import_keks[0x8];
> +	u8 num_nv_credentials[0x8];
> +	u8 reserved_at_60[0x3];
> +	u8 log_dek_granularity[0x5];
> +	u8 reserved_at_68[0x3];
> +	u8 log_max_num_int_kek[0x5];
> +	u8 reserved_at_70[0x10];
> +	u8 reserved_at_80[0x780];
> +};
> +
>  struct mlx5_ifc_crypto_commissioning_register_bits {
>  	u8 token[0x1]; /* TODO: add size after PRM update */
>  };
> --
> 2.21.0
> 
> 
> 
> ------------------------------
> 
> Message: 2
> Date: Mon, 18 Apr 2022 11:33:01 +0000
> From: Gagandeep Singh <G.Singh@nxp.com>
> To: Stephen Hemminger <stephen@networkplumber.org>, Ferruh Yigit
> 	<ferruh.yigit@xilinx.com>
> Cc: Harold Huang <baymaxhuang@gmail.com>, "dev@dpdk.org"
> 	<dev@dpdk.org>
> Subject: RE: [PATCH] kni: update kernel API to receive packets
> Message-ID:
> 	<AS8PR04MB8198A3AD1035DBE4BC9169D7E1F39@AS8PR04MB8198.e
> urprd04.prod.outlook.com>
> 
> Content-Type: text/plain; charset="us-ascii"
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, April 15, 2022 8:30 PM
> > To: Ferruh Yigit <ferruh.yigit@xilinx.com>
> > Cc: Gagandeep Singh <G.Singh@nxp.com>; Harold Huang
> > <baymaxhuang@gmail.com>; dev@dpdk.org
> > Subject: Re: [PATCH] kni: update kernel API to receive packets
> >
> > On Fri, 15 Apr 2022 13:30:33 +0100
> > Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
> >
> > > >> But this change would cause KNI kernel module does not work in the
> > > >> old kernel without this patch. I suggested using netif_rx_ni to keep
> > compatibility.
> > > >
> > > > netif_rx() API exists from very older versions of kernel before
> > > > v2.6. There will be no compilation issues. Only difference was,
> > > > netif_rx_ni() can be used in noninterrupt contexts to improve performance.
> > >
> > > May not be compilation issue, but with old kernels won't the behavior
> > > be different when 'netif_rx_ni()' switched to 'netif_rx()
> >
> > Probably best handled by #ifdef on kernel version but will be a mess for
> > backports to distro kernels.
> >
> > Looks like:
> >
> > 	Older	   -> New
> > 	netif_rx_ni   netif_rx
> > 	neitf_rx      __netif_rx
> 
> If it is ok for everyone, I can add #ifdef for kernel version >= 5.17 to use API
> netif_rx, to
> avoid any functional/performance impact with old kernels.
> 
> 
> ------------------------------
> 
> Message: 3
> Date: Mon, 18 Apr 2022 13:20:10 +0000
> From: "McDaniel, Timothy" <timothy.mcdaniel@intel.com>
> To: "dev@dpdk.org" <dev@dpdk.org>
> Subject: rte_bus_probe() does not exit on error
> Message-ID:
> 	<SN6PR11MB3103A5A4CE6D4C5846FDAB999EF39@SN6PR11MB3103.n
> amprd11.prod.outlook.com>
> 
> Content-Type: text/plain; charset="us-ascii"
> 
> Hello DPDK community,
> 
> We are looking into an issue where we pass an invalid command line argument
> to a vdev, and we print out an error message but don't exit.  This is an issue with
> all of the command line options that we handle in dlb2_parse_params().  In a
> nutshell, it looks like when we encounter an error in parsing, the error code is
> propagated to event_dlb2_vdev_probe() (event_dlb2_pci_probe() for PF), which
> is called by EAL during device probe in rte_bus_probe().  The problem is that
> rte_bus_probe() calls the .probe function for each device but doesn't exit on
> error:
> 
> if (vbus) {
>                 ret = vbus->probe();
>                 if (ret)
>                         RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
> }
> return 0;
> 
> We want to exit if the command line arguments can't be parsed, so we have a
> couple of options:
> 
> 
>   1.  In the DLB PMD, if we get an error while parsing parameters, exit right
> away.
>   2.  Change the behavior of rte_bus_probe() so that it propagates the error,
> which causes the program to exit due to EAL error.
> 
> if (vbus) {
>                 ret = vbus->probe();
>                 if (ret) {
>                         RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
>                         return ret;
>                 }
> }
> return 0;
> 
> Any preference, or other ideas?  Option 1 is simple and contained in our code,
> but does call rte_exit() from library code.  I'm not sure if that's really an issue
> because we do want to exit if the DLB-specific options are malformed.  Option 2
> is simple and seems like a better option, but requires changes outside of DLB, so
> may be more difficult to upstream?  (There may be reasons why the error is
> ignored for some devices).  Let me know what you think.
> 
> Thanks you for your comments,
> Tim McDaniel
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> <http://mails.dpdk.org/archives/dev/attachments/20220418/aa32380f/attachm
> ent.htm>
> 
> End of dev Digest, Vol 400, Issue 6
> ***********************************

Re: rte_bus_probe() does not exit on error

[Thomas Monjalon]

Hello,

02/05/2022 23:20, McDaniel, Timothy:
> Hello DPDK community,
> 
> I am following up on a question/comment that I submitted on April 18, for which
> I have not received any responses. See the original comment below for context.
> 
> Are there objections to modifying the behavior of rte_bus_probe() so that it propagates
> any errors detected while processing the command line arguments? It currently ignores
> errors and continues on, always returning success instead of any error that was returned
> by the probe function.

You are suggesting to stop if probing of one device fails.
I am not sure it is a good idea, because sometimes we are OK
to proceed even if one device is missing.

We could differentiate a fatal error like parsing syntax,
and "normal error" of a device which cannot be probed in some conditions.


> > -----Original Message-----
> > Hello DPDK community,
> > 
> > We are looking into an issue where we pass an invalid command line argument
> > to a vdev, and we print out an error message but don't exit.  This is an issue with
> > all of the command line options that we handle in dlb2_parse_params().  In a
> > nutshell, it looks like when we encounter an error in parsing, the error code is
> > propagated to event_dlb2_vdev_probe() (event_dlb2_pci_probe() for PF), which
> > is called by EAL during device probe in rte_bus_probe().  The problem is that
> > rte_bus_probe() calls the .probe function for each device but doesn't exit on
> > error:
> > 
> > if (vbus) {
> >                 ret = vbus->probe();
> >                 if (ret)
> >                         RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
> > }
> > return 0;
> > 
> > We want to exit if the command line arguments can't be parsed, so we have a
> > couple of options:
> > 
> > 
> >   1.  In the DLB PMD, if we get an error while parsing parameters, exit right
> > away.
> >   2.  Change the behavior of rte_bus_probe() so that it propagates the error,
> > which causes the program to exit due to EAL error.
> > 
> > if (vbus) {
> >                 ret = vbus->probe();
> >                 if (ret) {
> >                         RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
> >                         return ret;
> >                 }
> > }
> > return 0;
> > 
> > Any preference, or other ideas?  Option 1 is simple and contained in our code,
> > but does call rte_exit() from library code.  I'm not sure if that's really an issue
> > because we do want to exit if the DLB-specific options are malformed.  Option 2
> > is simple and seems like a better option, but requires changes outside of DLB, so
> > may be more difficult to upstream?  (There may be reasons why the error is
> > ignored for some devices).  Let me know what you think.
> > 
> > Thanks you for your comments,
> > Tim McDaniel

Re: rte_bus_probe() does not exit on error

[Tyler Retzlaff]

On Mon, May 02, 2022 at 11:54:29PM +0200, Thomas Monjalon wrote:
> Hello,
> 
> 02/05/2022 23:20, McDaniel, Timothy:
> > Hello DPDK community,
> > 
> > I am following up on a question/comment that I submitted on April 18, for which
> > I have not received any responses. See the original comment below for context.
> > 
> > Are there objections to modifying the behavior of rte_bus_probe() so that it propagates
> > any errors detected while processing the command line arguments? It currently ignores
> > errors and continues on, always returning success instead of any error that was returned
> > by the probe function.
> 
> You are suggesting to stop if probing of one device fails.
> I am not sure it is a good idea, because sometimes we are OK
> to proceed even if one device is missing.
> 
> We could differentiate a fatal error like parsing syntax,
> and "normal error" of a device which cannot be probed in some conditions.

a bit of a tangent but it would be nice if eal initialization wasn't
coupled to bus/device enumeration at all and instead there was more
control over bus/device enumeration where the application could choose if
it wants the error to be fatal or not .. after eal was initialized.

with it burried inside eal initialization the application has no control
over the policy to fail or not, also there are other peripherial
problems that arise due to the composition e.g. event callbacks can't
be registered until after probe from init has occurred and eal init
is completed.

it would be a huge compat break (i'm ignoring that) but so would
failing eal init for reasons it does not currently fail.

Re: rte_bus_probe() does not exit on error

[Thomas Monjalon]

03/05/2022 11:52, Tyler Retzlaff:
> On Mon, May 02, 2022 at 11:54:29PM +0200, Thomas Monjalon wrote:
> > Hello,
> > 
> > 02/05/2022 23:20, McDaniel, Timothy:
> > > Hello DPDK community,
> > > 
> > > I am following up on a question/comment that I submitted on April 18, for which
> > > I have not received any responses. See the original comment below for context.
> > > 
> > > Are there objections to modifying the behavior of rte_bus_probe() so that it propagates
> > > any errors detected while processing the command line arguments? It currently ignores
> > > errors and continues on, always returning success instead of any error that was returned
> > > by the probe function.
> > 
> > You are suggesting to stop if probing of one device fails.
> > I am not sure it is a good idea, because sometimes we are OK
> > to proceed even if one device is missing.
> > 
> > We could differentiate a fatal error like parsing syntax,
> > and "normal error" of a device which cannot be probed in some conditions.
> 
> a bit of a tangent but it would be nice if eal initialization wasn't
> coupled to bus/device enumeration at all and instead there was more
> control over bus/device enumeration where the application could choose if
> it wants the error to be fatal or not .. after eal was initialized.

I agree with the idea.

> with it burried inside eal initialization the application has no control
> over the policy to fail or not, also there are other peripherial
> problems that arise due to the composition e.g. event callbacks can't
> be registered until after probe from init has occurred and eal init
> is completed.
> 
> it would be a huge compat break (i'm ignoring that) but so would
> failing eal init for reasons it does not currently fail.

Yes compatibility is a blocker.

A better idea would be to not use rte_eal_init() at all.
I am convinced we should split this function in multiple parts.
It would allow keeping compatibility with the legacy function
while allowing more flexibility with new functions.

You may be interested by this talk:
https://fast.dpdk.org/events/slides/DPDK-2018-09-Default.pdf

RE: rte_bus_probe() does not exit on error

[McDaniel, Timothy]

Thanks for the reply, Thomas.  For us, our request is to terminate if a command
line syntax error is detected.  I understand that this would break backward 
compatibility, so perhaps we can look at addressing the issue  at the next appropriate
release.

Thanks,
Tim


> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, May 3, 2022 5:14 AM
> To: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Cc: McDaniel, Timothy <timothy.mcdaniel@intel.com>; dev@dpdk.org; Van
> Haaren, Harry <harry.van.haaren@intel.com>; Jerin Jacob
> <jerinj@marvell.com>; Wires, Kent <kent.wires@intel.com>;
> david.marchand@redhat.com
> Subject: Re: rte_bus_probe() does not exit on error
> 
> 03/05/2022 11:52, Tyler Retzlaff:
> > On Mon, May 02, 2022 at 11:54:29PM +0200, Thomas Monjalon wrote:
> > > Hello,
> > >
> > > 02/05/2022 23:20, McDaniel, Timothy:
> > > > Hello DPDK community,
> > > >
> > > > I am following up on a question/comment that I submitted on April 18, for
> which
> > > > I have not received any responses. See the original comment below for
> context.
> > > >
> > > > Are there objections to modifying the behavior of rte_bus_probe() so that
> it propagates
> > > > any errors detected while processing the command line arguments? It
> currently ignores
> > > > errors and continues on, always returning success instead of any error that
> was returned
> > > > by the probe function.
> > >
> > > You are suggesting to stop if probing of one device fails.
> > > I am not sure it is a good idea, because sometimes we are OK
> > > to proceed even if one device is missing.
> > >
> > > We could differentiate a fatal error like parsing syntax,
> > > and "normal error" of a device which cannot be probed in some conditions.
> >
> > a bit of a tangent but it would be nice if eal initialization wasn't
> > coupled to bus/device enumeration at all and instead there was more
> > control over bus/device enumeration where the application could choose if
> > it wants the error to be fatal or not .. after eal was initialized.
> 
> I agree with the idea.
> 
> > with it burried inside eal initialization the application has no control
> > over the policy to fail or not, also there are other peripherial
> > problems that arise due to the composition e.g. event callbacks can't
> > be registered until after probe from init has occurred and eal init
> > is completed.
> >
> > it would be a huge compat break (i'm ignoring that) but so would
> > failing eal init for reasons it does not currently fail.
> 
> Yes compatibility is a blocker.
> 
> A better idea would be to not use rte_eal_init() at all.
> I am convinced we should split this function in multiple parts.
> It would allow keeping compatibility with the legacy function
> while allowing more flexibility with new functions.
> 
> You may be interested by this talk:
> https://fast.dpdk.org/events/slides/DPDK-2018-09-Default.pdf
> 

rte_bus_probe() does not exit on error

[McDaniel, Timothy]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hello DPDK community,

We are looking into an issue where we pass an invalid command line argument to a vdev, and we print out an error message but don't exit.  This is an issue with all of the command line options that we handle in dlb2_parse_params().  In a nutshell, it looks like when we encounter an error in parsing, the error code is propagated to event_dlb2_vdev_probe() (event_dlb2_pci_probe() for PF), which is called by EAL during device probe in rte_bus_probe().  The problem is that rte_bus_probe() calls the .probe function for each device but doesn't exit on error:

if (vbus) {
                ret = vbus->probe();
                if (ret)
                        RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
}
return 0;

We want to exit if the command line arguments can't be parsed, so we have a couple of options:


  1.  In the DLB PMD, if we get an error while parsing parameters, exit right away.
  2.  Change the behavior of rte_bus_probe() so that it propagates the error, which causes the program to exit due to EAL error.

if (vbus) {
                ret = vbus->probe();
                if (ret) {
                        RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
                        return ret;
                }
}
return 0;

Any preference, or other ideas?  Option 1 is simple and contained in our code, but does call rte_exit() from library code.  I'm not sure if that's really an issue because we do want to exit if the DLB-specific options are malformed.  Option 2 is simple and seems like a better option, but requires changes outside of DLB, so may be more difficult to upstream?  (There may be reasons why the error is ignored for some devices).  Let me know what you think.

Thanks you for your comments,
Tim McDaniel

[-- Attachment #2: Type: text/html, Size: 6927 bytes --]