From: Dariusz Sosnowski <dsosnowski@nvidia.com>
To: Konstantin Ananyev <konstantin.ananyev@huawei.com>,
"NBU-Contact-Thomas Monjalon (EXTERNAL)" <thomas@monjalon.net>,
Ferruh Yigit <ferruh.yigit@amd.com>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v2 2/4] ethdev: add get restore flags driver callback
Date: Fri, 11 Oct 2024 15:30:39 +0000 [thread overview]
Message-ID: <CH3PR12MB846024AEAEDA90D5CED41728A4792@CH3PR12MB8460.namprd12.prod.outlook.com> (raw)
In-Reply-To: <36888a4e9c074e06a822ca84a2eda09b@huawei.com>
> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Sent: Friday, October 11, 2024 14:54
> To: Dariusz Sosnowski <dsosnowski@nvidia.com>; NBU-Contact-Thomas
> Monjalon (EXTERNAL) <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@amd.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 2/4] ethdev: add get restore flags driver callback
>
> External email: Use caution opening links or attachments
>
>
> > Before this patch, ethdev layer assumed that all drivers require that
> > it has to forcefully restore:
> >
> > - MAC addresses
> > - promiscuous mode setting
> > - all multicast mode setting
> >
> > upon rte_eth_dev_start().
> >
> > This patch introduces a new callback to eth_dev_ops -
> > get_restore_flags().
> > Drivers implementing this callback can explicitly enable/disable
> > certain parts of config restore procedure.
> >
> > In order to minimize the changes to all the drivers and preserve the
> > current behavior, it is assumed that if this callback is not defined,
> > all configuration should be restored.
> >
> > Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
>
> LGTM, just few nits/suggestions, pls see below.
> Series-Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
>
> > ---
> > lib/ethdev/ethdev_driver.c | 11 +++++++ lib/ethdev/ethdev_driver.h |
> > 64 ++++++++++++++++++++++++++++++++++++++
> > lib/ethdev/version.map | 1 +
> > 3 files changed, 76 insertions(+)
> >
> > diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> > index c335a25a82..f78f9fb5c1 100644
> > --- a/lib/ethdev/ethdev_driver.c
> > +++ b/lib/ethdev/ethdev_driver.c
> > @@ -958,3 +958,14 @@ rte_eth_switch_domain_free(uint16_t domain_id)
> >
> > return 0;
> > }
> > +
> > +void
> > +rte_eth_get_restore_flags(struct rte_eth_dev *dev,
> > + enum rte_eth_dev_operation op,
> > + uint32_t *flags)
>
> I would go with uint64_t for flags (to avoid limitations in future).
+1
> Also, If it is void anyway, might be just return flags?
> i.e:
> uint64_t +rte_eth_get_restore_flags(struct rte_eth_dev *dev, enum
> rte_eth_dev_operation op);
+1
>
> Another thing - do we need to do update docs?
> PMD or PG sections, release notes?
IIRC new additions and internal changes in ethdev are not advertised in release notes.
@Ferruh Yigit: Should these changes be documented/announced somewhere?
>
> > +{
> > + if (dev->dev_ops->get_restore_flags != NULL)
> > + dev->dev_ops->get_restore_flags(dev, op, flags);
> > + else
> > + *flags = RTE_ETH_RESTORE_ALL; }
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index ae00ead865..8ac5328521 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1235,6 +1235,47 @@ typedef int (*eth_count_aggr_ports_t)(struct
> > rte_eth_dev *dev); typedef int (*eth_map_aggr_tx_affinity_t)(struct
> rte_eth_dev *dev, uint16_t tx_queue_id,
> > uint8_t affinity);
> >
> > +/**
> > + * @internal
> > + * Defines types of operations which can be executed by the application.
> > + */
> > +enum rte_eth_dev_operation {
> > + RTE_ETH_START,
> > +};
> > +
> > +/**@{@name Restore flags
> > + * Flags returned by get_restore_flags() callback.
> > + * They indicate to ethdev layer which configuration is required to be restored.
> > + */
> > +/** If set, ethdev layer will forcefully restore default and any
> > +other added MAC addresses. */ #define RTE_ETH_RESTORE_MAC_ADDR
> > +RTE_BIT32(0)
> > +/** If set, ethdev layer will forcefully restore current promiscuous
> > +mode setting. */ #define RTE_ETH_RESTORE_PROMISC RTE_BIT32(1)
> > +/** If set, ethdev layer will forcefully restore current all
> > +multicast mode setting. */ #define RTE_ETH_RESTORE_ALLMULTI
> > +RTE_BIT32(2) /**@}*/
> > +
> > +/** All configuration which can be restored by ethdev layer. */
> > +#define RTE_ETH_RESTORE_ALL (RTE_ETH_RESTORE_MAC_ADDR | \
> > + RTE_ETH_RESTORE_PROMISC | \
> > + RTE_ETH_RESTORE_ALLMULTI)
> > +
> > +/**
> > + * @internal
> > + * Fetch from the driver what kind of configuration must be restored
> > +by ethdev layer,
> > + * after certain operations are performed by the application (such as
> rte_eth_dev_start()).
> > + *
> > + * @param dev
> > + * Port (ethdev) handle.
> > + * @param op
> > + * Type of operation executed by the application.
> > + * @param flags
> > + * Flags indicating what configuration must be restored by ethdev layer.
> > + */
> > +typedef void (*eth_get_restore_flags_t)(struct rte_eth_dev *dev,
> > + enum rte_eth_dev_operation op,
> > + uint32_t *flags);
> > +
> > /**
> > * @internal A structure containing the functions exported by an Ethernet
> driver.
> > */
> > @@ -1474,6 +1515,9 @@ struct eth_dev_ops {
> > eth_count_aggr_ports_t count_aggr_ports;
> > /** Map a Tx queue with an aggregated port of the DPDK port */
> > eth_map_aggr_tx_affinity_t map_aggr_tx_affinity;
> > +
> > + /** Get configuration which ethdev should restore */
> > + eth_get_restore_flags_t get_restore_flags;
> > };
> >
> > /**
> > @@ -2131,6 +2175,26 @@ struct rte_eth_fdir_conf {
> > struct rte_eth_fdir_flex_conf flex_conf; };
> >
> > +/**
> > + * @internal
> > + * Fetch from the driver what kind of configuration must be restored
> > +by ethdev layer,
> > + * using get_restore_flags() callback.
> > + *
> > + * If callback is not defined, it is assumed that all supported configuration must
> be restored.
> > + *
> > + * @param dev
> > + * Port (ethdev) handle.
> > + * @param op
> > + * Type of operation executed by the application.
> > + * @param flags
> > + * Flags indicating what configuration must be restored by ethdev layer.
> > + */
> > +__rte_internal
>
> For new function we probably need 'rte_experimental'...
> But from other side - it is internal.
> Not sure what are rules here for such case.
>
> > +void
> > +rte_eth_get_restore_flags(struct rte_eth_dev *dev,
> > + enum rte_eth_dev_operation op,
> > + uint32_t *flags);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index
> > 1669055ca5..fa0469e602 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -358,4 +358,5 @@ INTERNAL {
> > rte_eth_switch_domain_alloc;
> > rte_eth_switch_domain_free;
> > rte_flow_fp_default_ops;
> > + rte_eth_get_restore_flags;
> > };
> > --
> > 2.39.5
next prev parent reply other threads:[~2024-10-11 15:30 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-11 9:20 [PATCH 0/4] ethdev: rework config restore Dariusz Sosnowski
2024-10-11 9:21 ` [PATCH 1/4] " Dariusz Sosnowski
2024-10-11 9:21 ` [PATCH 2/4] ethdev: add get restore flags driver callback Dariusz Sosnowski
2024-10-11 9:21 ` [PATCH 3/4] ethdev: restore config only when requested Dariusz Sosnowski
2024-10-11 9:21 ` [PATCH 4/4] net/mlx5: disable config restore Dariusz Sosnowski
2024-10-11 9:33 ` [PATCH v2 0/4] ethdev: rework " Dariusz Sosnowski
2024-10-11 9:33 ` [PATCH v2 1/4] " Dariusz Sosnowski
2024-10-11 9:33 ` [PATCH v2 2/4] ethdev: add get restore flags driver callback Dariusz Sosnowski
2024-10-11 12:54 ` Konstantin Ananyev
2024-10-11 15:30 ` Dariusz Sosnowski [this message]
2024-10-11 9:33 ` [PATCH v2 3/4] ethdev: restore config only when requested Dariusz Sosnowski
2024-10-11 9:33 ` [PATCH v2 4/4] net/mlx5: disable config restore Dariusz Sosnowski
2024-10-11 18:51 ` [PATCH v3 0/4] ethdev: rework " Dariusz Sosnowski
2024-10-11 18:51 ` [PATCH v3 1/4] " Dariusz Sosnowski
2024-10-11 18:51 ` [PATCH v3 2/4] ethdev: add get restore flags driver callback Dariusz Sosnowski
2024-10-11 18:51 ` [PATCH v3 3/4] ethdev: restore config only when requested Dariusz Sosnowski
2024-10-11 18:51 ` [PATCH v3 4/4] net/mlx5: disable config restore Dariusz Sosnowski
2024-10-11 21:25 ` [PATCH v3 0/4] ethdev: rework " Ferruh Yigit
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=CH3PR12MB846024AEAEDA90D5CED41728A4792@CH3PR12MB8460.namprd12.prod.outlook.com \
--to=dsosnowski@nvidia.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=konstantin.ananyev@huawei.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).