DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ray Kinsella <mdr@ashroe.eu>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: Ferruh Yigit <ferruh.yigit@intel.com>,
	Kalesh A P <kalesh-anakkur.purayil@broadcom.com>,
	dev@dpdk.org, ajit.khaparde@broadcom.com, asafp@nvidia.com,
	David Marchand <david.marchand@redhat.com>,
	Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Subject: Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events
Date: Mon, 14 Feb 2022 11:06:05 -0500	[thread overview]
Message-ID: <875yphigb6.fsf@mdr78.vserver.site> (raw)
In-Reply-To: <45691978.XUcTiDjVJD@thomas>


Thomas Monjalon <thomas@monjalon.net> writes:

> 14/02/2022 11:16, Ray Kinsella:
>> Ray Kinsella <mdr@ashroe.eu> writes:
>> > Thomas Monjalon <thomas@monjalon.net> writes:
>> >> 02/02/2022 12:44, Ray Kinsella:
>> >>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>> >>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
>> >>> >> --- a/lib/ethdev/rte_ethdev.h
>> >>> >> +++ b/lib/ethdev/rte_ethdev.h
>> >>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>> >>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>> >>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>> >>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>> >>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> >>> >> +			/**< port recovering from an error
>> >>> >> +			 *
>> >>> >> +			 * PMD detected a FW reset or error condition.
>> >>> >> +			 * PMD will try to recover from the error.
>> >>> >> +			 * Data path may be quiesced and Control path operations
>> >>> >> +			 * may fail at this time.
>> >>> >> +			 */
>> >>> >> +	RTE_ETH_EVENT_RECOVERED,
>> >>> >> +			/**< port recovered from an error
>> >>> >> +			 *
>> >>> >> +			 * PMD has recovered from the error condition.
>> >>> >> +			 * Control path and Data path are up now.
>> >>> >> +			 * PMD re-configures the port to the state prior to the error.
>> >>> >> +			 * Since the device has undergone a reset, flow rules
>> >>> >> +			 * offloaded prior to reset may be lost and
>> >>> >> +			 * the application should recreate the rules again.
>> >>> >> +			 */
>> >>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>> >>> >
>> >>> >
>> >>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>> >>> > to evaluate if it is a false positive:
>> >>> >
>> >>> >
>> >>> > 1 function with some indirect sub-type change:
>> >>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>> >>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>> >>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>> >>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>> >>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>> >>> >             type size hasn't changed
>> >>> >             2 enumerator insertions:
>> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>> >>> >             1 enumerator change:
>> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>> >>> 
>> >>> I don't immediately see the problem that this would cause.
>> >>> There are no array sizes etc dependent on the value of MAX for instance.
>> >>> 
>> >>> Looks safe?
>> >>
>> >> We never know how this enum will be used by the application.
>> >> The max value may be used for the size of an event array.
>> >> It looks a real ABI issue unfortunately.
>> >
>> > Right - but we only really care about it when an array size based on MAX
>> > is likely to be passed to DPDK, which doesn't apply in this case.
>
> I don't completely agree.
> A developer may assume an event will never exceed MAX value.
> However, after an upgrade of DPDK without app rebuild,
> a higher event value may be received in the app,
> breaking the assumption.
> Should we consider this case as an ABI breakage?

Nope - I think we should explicitly exclude MAX values from any
ABI guarantee, as being able to change them is key to our be able to
evolve DPDK while maintaining ABI stability. 

Consider what it means applying the ABI policy to a MAX value, you are
in effect saying that that no value can be added to this enumeration
until the next ABI version, for me this is very restrictive without a
solid reason. 

>
>> > I noted that some Linux folks explicitly mark similar MAX values as not
>> > part of the ABI.
>> >
>> > /usr/include/linux/perf_event.h
>> > 37:     PERF_TYPE_MAX,                          /* non-ABI */
>> > 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>> > 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>> > 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>> > 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>> > 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>> > 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>> > 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>> > non-ABI; internal use */
>> > 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>> > 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>> > 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>> > 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>> > 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>> > 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>> 
>> Any thoughts on similarly annotating all our _MAX enums in the same way?
>> We could also add a section in the ABI Policy to make it explicit _MAX
>> enum values are not part of the ABI - what do folks think?
>
> Interesting. I am not sure it is always ABI-safe though.


-- 
Regards, Ray K

  reply	other threads:[~2022-02-14 16:06 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-22 10:16 [dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support Kalesh A P
2020-01-22 10:16 ` [dpdk-dev] [RFC PATCH 1/3] librte_ethdev: support device recovery event Kalesh A P
2020-03-11 13:20   ` Thomas Monjalon
2020-03-12  3:31     ` Kalesh Anakkur Purayil
2020-03-12  7:29       ` Thomas Monjalon
2020-01-22 10:16 ` [dpdk-dev] [RFC PATCH 2/3] net/bnxt: notify applications about device reset Kalesh A P
2020-01-22 10:16 ` [dpdk-dev] [RFC PATCH 3/3] app/testpmd: handle device recovery event Kalesh A P
2020-03-11 13:19 ` [dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support Thomas Monjalon
2020-03-12  3:25   ` Kalesh Anakkur Purayil
2020-03-12  7:34     ` Thomas Monjalon
2020-07-03 16:12       ` Ferruh Yigit
2020-09-30  7:03 ` [dpdk-dev] [RFC V2 " Kalesh A P
2020-09-30  7:03   ` [dpdk-dev] [RFC V2 1/3] ethdev: support device reset and recovery events Kalesh A P
2020-09-30  7:03   ` [dpdk-dev] [RFC V2 2/3] net/bnxt: notify applications about device reset/recovery Kalesh A P
2020-09-30  7:03   ` [dpdk-dev] [RFC V2 3/3] app/testpmd: handle device recovery event Kalesh A P
2020-09-30  7:07 ` [dpdk-dev] [RFC PATCH v2 0/3] librte_ethdev: error recovery support Kalesh A P
2020-09-30  7:07   ` [dpdk-dev] [RFC PATCH v2 1/3] ethdev: support device reset and recovery events Kalesh A P
2020-09-30  7:07   ` [dpdk-dev] [RFC PATCH v2 2/3] net/bnxt: notify applications about device reset/recovery Kalesh A P
2020-09-30  7:07   ` [dpdk-dev] [RFC PATCH v2 3/3] app/testpmd: handle device recovery event Kalesh A P
2020-09-30  7:12 ` [dpdk-dev] [RFC PATCH v3 0/3] librte_ethdev: error recovery support Kalesh A P
2020-09-30  7:12   ` [dpdk-dev] [RFC PATCH v3 1/3] ethdev: support device reset and recovery events Kalesh A P
2020-09-30  7:50     ` Thomas Monjalon
2020-09-30  8:35       ` Kalesh Anakkur Purayil
2020-09-30  9:31         ` Thomas Monjalon
2020-09-30  7:12   ` [dpdk-dev] [RFC PATCH v3 2/3] net/bnxt: notify applications about device reset/recovery Kalesh A P
2020-09-30  7:12   ` [dpdk-dev] [RFC PATCH v3 3/3] app/testpmd: handle device recovery event Kalesh A P
2020-10-08 10:53     ` Asaf Penso
2020-09-30 12:33 ` [dpdk-dev] [RFC PATCH v4 0/3] librte_ethdev: error recovery support Kalesh A P
2020-09-30 12:33   ` [dpdk-dev] [RFC PATCH v4 1/3] ethdev: support device reset and recovery events Kalesh A P
2020-09-30 12:33   ` [dpdk-dev] [RFC PATCH v4 2/3] net/bnxt: notify applications about device reset/recovery Kalesh A P
2020-09-30 12:33   ` [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event Kalesh A P
2020-10-06 17:25     ` Ophir Munk
2020-10-07  4:46       ` Kalesh Anakkur Purayil
2020-10-07  8:36         ` Ophir Munk
2020-10-07  9:37         ` Ferruh Yigit
2020-10-07 18:42           ` Ajit Khaparde
2020-10-07 16:49 ` [dpdk-dev] [PATCH v5 0/3] librte_ethdev: error recovery support Kalesh A P
2020-10-07 16:49   ` [dpdk-dev] [PATCH v5 1/3] ethdev: support device reset and recovery events Kalesh A P
2020-10-08 10:49     ` Asaf Penso
2020-10-07 16:49   ` [dpdk-dev] [PATCH v5 2/3] net/bnxt: notify applications about device reset/recovery Kalesh A P
2020-10-07 16:49   ` [dpdk-dev] [PATCH v5 3/3] app/testpmd: handle device recovery event Kalesh A P
2020-10-09  3:48 ` [dpdk-dev] [PATCH v6 0/3] librte_ethdev: error recovery support Kalesh A P
2020-10-09  3:48   ` [dpdk-dev] [PATCH v6 1/3] ethdev: support device reset and recovery events Kalesh A P
2020-10-11 21:29     ` Thomas Monjalon
2020-10-12  8:09       ` Andrew Rybchenko
2021-02-18 15:32         ` Ferruh Yigit
2020-10-09  3:48   ` [dpdk-dev] [PATCH v6 2/3] net/bnxt: notify applications about device reset/recovery Kalesh A P
2020-10-09  3:48   ` [dpdk-dev] [PATCH v6 3/3] app/testpmd: handle device recovery event Kalesh A P
2022-01-28 12:48   ` [dpdk-dev] [PATCH v7 0/4] ethdev: error recovery support Kalesh A P
2022-01-28 12:48     ` [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events Kalesh A P
2022-02-01 12:11       ` Ferruh Yigit
2022-02-01 13:09         ` Kalesh Anakkur Purayil
2022-02-01 13:19           ` Ferruh Yigit
2022-02-03 20:28             ` Ajit Khaparde
2022-02-10 22:42               ` Thomas Monjalon
2022-02-01 12:52       ` Ferruh Yigit
2022-02-02 11:44         ` Ray Kinsella
2022-02-10 22:16           ` Thomas Monjalon
2022-02-11 10:09             ` Ray Kinsella
2022-02-14 10:16               ` Ray Kinsella
2022-02-14 11:15                 ` Thomas Monjalon
2022-02-14 16:06                   ` Ray Kinsella [this message]
2022-02-14 16:25                     ` Thomas Monjalon
2022-02-14 18:27                       ` Ray Kinsella
2022-02-15 13:55                         ` Ray Kinsella
2022-02-15 15:12                           ` Thomas Monjalon
2022-02-15 16:12                             ` Ray Kinsella
2022-05-21 10:33                     ` fengchengwen
2022-05-24 15:11                       ` Ray Kinsella
2022-06-10  0:16                         ` fengchengwen
2022-01-28 12:48     ` [dpdk-dev] [PATCH v7 2/4] app/testpmd: handle device recovery event Kalesh A P
2022-01-28 12:48     ` [dpdk-dev] [PATCH v7 3/4] net/bnxt: notify applications about device reset/recovery Kalesh A P
2022-01-28 12:48     ` [dpdk-dev] [PATCH v7 4/4] doc: update release notes Kalesh A P
2022-02-01 12:12       ` Ferruh Yigit
2022-06-16  9:41     ` [PATCH v8 0/4] ethdev: support error recovery notification Chengwen Feng
2022-06-16  9:41       ` [PATCH v8 1/4] ethdev: support device " Chengwen Feng
2022-06-20 17:42         ` Thomas Monjalon
2022-06-21  1:38           ` fengchengwen
2022-06-21  7:04             ` Thomas Monjalon
2022-09-22  7:53               ` fengchengwen
2022-06-23 15:58         ` Ray Kinsella
2022-06-16  9:41       ` [PATCH v8 2/4] app/testpmd: handle error recovery notification event Chengwen Feng
2022-06-16  9:41       ` [PATCH v8 3/4] net/hns3: support " Chengwen Feng
2022-06-16  9:41       ` [PATCH v8 4/4] net/bnxt: notify applications about device reset/recovery Chengwen Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875yphigb6.fsf@mdr78.vserver.site \
    --to=mdr@ashroe.eu \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=asafp@nvidia.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=kalesh-anakkur.purayil@broadcom.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).