[dpdk-dev,06/11] ethdev: allow ownership operations on unused port
Checks
Commit Message
From: Matan Azrad <matan@mellanox.com>
When the state will be updated later than in allocation,
we may need to update the ownership of a port which is
still in state unused.
It will be used to take ownership of a port before it is
declared as available for other entities.
Cc: stable@dpdk.org
Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
lib/librte_ethdev/rte_ethdev.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
Comments
On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
>
> When the state will be updated later than in allocation,
> we may need to update the ownership of a port which is
> still in state unused.
>
> It will be used to take ownership of a port before it is
> declared as available for other entities.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
Hi Thomas,
If I remember correctly port ownership merged last release in last minute,
without much review. Now we are having these updates on it on rc3, not sure if
many people aware of this feature.
Instead of getting these updates in rc3, any chance to postpone to next release
and do more reviews?
Thanks,
ferruh
09/05/2018 20:00, Ferruh Yigit:
> On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> > From: Matan Azrad <matan@mellanox.com>
> >
> > When the state will be updated later than in allocation,
> > we may need to update the ownership of a port which is
> > still in state unused.
> >
> > It will be used to take ownership of a port before it is
> > declared as available for other entities.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
>
> Hi Thomas,
>
> If I remember correctly port ownership merged last release in last minute,
> without much review. Now we are having these updates on it on rc3, not sure if
> many people aware of this feature.
The main purpose is about fixing events NEW and DESTROY.
> Instead of getting these updates in rc3, any chance to postpone to next release
> and do more reviews?
They are not updates but fixes, I think we must consider them.
On Wed, 9 May 2018 11:43:32 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:
> - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {
Since name being empty now has significance, why not introduce an macro or inline function
to make the test. Also, static checkers don't like pointers which maybe outside valid
range (sometimes).
static inline bool rte_ethdev_is_unused(const struct rte_ethdev *ethdev)
{
return ethdev->data->name[0] == '\0';
}
#define RTE_ETH_UNUSED_OR_ERR_RET(ethdev, retval) do { \
if (!rte_ethdev_is_unused(ethdev)) { \
RTE_PMD_DEBUG_TRACE("Port already in use=%d\n", ethdev->port_id); \
return retval; \
} } while(0)
10/05/2018 22:26, Stephen Hemminger:
> On Wed, 9 May 2018 11:43:32 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
>
> > - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > + if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {
>
> Since name being empty now has significance, why not introduce an macro or inline function
> to make the test. Also, static checkers don't like pointers which maybe outside valid
> range (sometimes).
>
> static inline bool rte_ethdev_is_unused(const struct rte_ethdev *ethdev)
> {
> return ethdev->data->name[0] == '\0';
> }
>
> #define RTE_ETH_UNUSED_OR_ERR_RET(ethdev, retval) do { \
> if (!rte_ethdev_is_unused(ethdev)) { \
> RTE_PMD_DEBUG_TRACE("Port already in use=%d\n", ethdev->port_id); \
> return retval; \
> } } while(0)
Yes we can have a static function for this check.
The name should be "ethdev_is_allocated".
@@ -414,10 +414,14 @@ static int
_rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
const struct rte_eth_dev_owner *new_owner)
{
+ struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
struct rte_eth_dev_owner *port_owner;
int sret;
- RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {
+ RTE_PMD_DEBUG_TRACE("Port id %"PRIu16" is not allocated.\n", port_id);
+ return -ENODEV;
+ }
if (!rte_eth_is_valid_owner_id(new_owner->id) &&
!rte_eth_is_valid_owner_id(old_owner_id))
@@ -481,16 +485,17 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
void __rte_experimental
rte_eth_dev_owner_delete(const uint64_t owner_id)
{
- uint16_t port_id;
+ uint32_t port_id;
rte_eth_dev_shared_data_prepare();
rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
if (rte_eth_is_valid_owner_id(owner_id)) {
- RTE_ETH_FOREACH_DEV_OWNED_BY(port_id, owner_id)
- memset(&rte_eth_devices[port_id].data->owner, 0,
- sizeof(struct rte_eth_dev_owner));
+ for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++)
+ if (rte_eth_devices[port_id].data->owner.id == owner_id)
+ memset(&rte_eth_devices[port_id].data->owner, 0,
+ sizeof(struct rte_eth_dev_owner));
RTE_PMD_DEBUG_TRACE("All port owners owned by %016"PRIX64
" identifier have removed.\n", owner_id);
}
@@ -502,17 +507,17 @@ int __rte_experimental
rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
{
int ret = 0;
+ struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
rte_eth_dev_shared_data_prepare();
rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
- if (!rte_eth_dev_is_valid_port(port_id)) {
- RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+ if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {
+ RTE_PMD_DEBUG_TRACE("Port id %"PRIu16" is not allocated.\n", port_id);
ret = -ENODEV;
} else {
- rte_memcpy(owner, &rte_eth_devices[port_id].data->owner,
- sizeof(*owner));
+ rte_memcpy(owner, ðdev->data->owner, sizeof(*owner));
}
rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);