eventdev: fix device probe and remove for secondary process
diff mbox series

Message ID 20200427181039.1943-1-pbhagavatula@marvell.com
State Accepted, archived
Delegated to: Jerin Jacob
Headers show
Series
  • eventdev: fix device probe and remove for secondary process
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-intel-Performance fail Performance Testing issues
ci/travis-robot warning Travis build: failed
ci/checkpatch success coding style OK

Commit Message

Pavan Nikhilesh Bhagavatula April 27, 2020, 6:10 p.m. UTC
From: Pavan Nikhilesh <pbhagavatula@marvell.com>

When probing event device in secondary process skip reinitializing
the device data structure as it is already done in primary process.

When removing event device in secondary process skip closing the
event device as it should be done by primary process.

Fixes: 322d0345c2bc ("eventdev: implement PMD registration functions")

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 lib/librte_eventdev/rte_eventdev.c         | 13 ++++++++-----
 lib/librte_eventdev/rte_eventdev_pmd_pci.h |  8 +++++---
 2 files changed, 13 insertions(+), 8 deletions(-)

Comments

Jerin Jacob May 1, 2020, 1:26 p.m. UTC | #1
On Mon, Apr 27, 2020 at 11:40 PM <pbhagavatula@marvell.com> wrote:
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> When probing event device in secondary process skip reinitializing
> the device data structure as it is already done in primary process.
>
> When removing event device in secondary process skip closing the
> event device as it should be done by primary process.
>
> Fixes: 322d0345c2bc ("eventdev: implement PMD registration functions")

Cc: stable@dpdk.org

>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>


Acked-by: Jerin Jacob <jerinj@marvell.com>



> ---
>  lib/librte_eventdev/rte_eventdev.c         | 13 ++++++++-----
>  lib/librte_eventdev/rte_eventdev_pmd_pci.h |  8 +++++---
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
> index b987e0745..9aca7fbd5 100644
> --- a/lib/librte_eventdev/rte_eventdev.c
> +++ b/lib/librte_eventdev/rte_eventdev.c
> @@ -1364,14 +1364,17 @@ rte_event_pmd_allocate(const char *name, int socket_id)
>
>                 eventdev->data = eventdev_data;
>
> -               strlcpy(eventdev->data->name, name, RTE_EVENTDEV_NAME_MAX_LEN);
> +               if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>
> -               eventdev->data->dev_id = dev_id;
> -               eventdev->data->socket_id = socket_id;
> -               eventdev->data->dev_started = 0;
> +                       strlcpy(eventdev->data->name, name,
> +                               RTE_EVENTDEV_NAME_MAX_LEN);
>
> -               eventdev->attached = RTE_EVENTDEV_ATTACHED;
> +                       eventdev->data->dev_id = dev_id;
> +                       eventdev->data->socket_id = socket_id;
> +                       eventdev->data->dev_started = 0;
> +               }
>
> +               eventdev->attached = RTE_EVENTDEV_ATTACHED;
>                 eventdev_globals.nb_devs++;
>         }
>
> diff --git a/lib/librte_eventdev/rte_eventdev_pmd_pci.h b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> index 8fb61386f..443cd38c2 100644
> --- a/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> +++ b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> @@ -112,9 +112,11 @@ rte_event_pmd_pci_remove(struct rte_pci_device *pci_dev,
>         if (eventdev == NULL)
>                 return -ENODEV;
>
> -       ret = rte_event_dev_close(eventdev->data->dev_id);
> -       if (ret < 0)
> -               return ret;
> +       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +               ret = rte_event_dev_close(eventdev->data->dev_id);
> +               if (ret < 0)
> +                       return ret;
> +       }
>
>         /* Invoke PMD device un-init function */
>         if (devuninit)
> --
> 2.17.1
>
Jerin Jacob May 2, 2020, 10:31 a.m. UTC | #2
On Fri, May 1, 2020 at 6:56 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Mon, Apr 27, 2020 at 11:40 PM <pbhagavatula@marvell.com> wrote:
> >
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > When probing event device in secondary process skip reinitializing
> > the device data structure as it is already done in primary process.
> >
> > When removing event device in secondary process skip closing the
> > event device as it should be done by primary process.
> >
> > Fixes: 322d0345c2bc ("eventdev: implement PMD registration functions")
>
> Cc: stable@dpdk.org
>
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
>
> Acked-by: Jerin Jacob <jerinj@marvell.com>


Applied to dpdk-next-eventdev/master. Thanks.


>
>
>
> > ---
> >  lib/librte_eventdev/rte_eventdev.c         | 13 ++++++++-----
> >  lib/librte_eventdev/rte_eventdev_pmd_pci.h |  8 +++++---
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
> > index b987e0745..9aca7fbd5 100644
> > --- a/lib/librte_eventdev/rte_eventdev.c
> > +++ b/lib/librte_eventdev/rte_eventdev.c
> > @@ -1364,14 +1364,17 @@ rte_event_pmd_allocate(const char *name, int socket_id)
> >
> >                 eventdev->data = eventdev_data;
> >
> > -               strlcpy(eventdev->data->name, name, RTE_EVENTDEV_NAME_MAX_LEN);
> > +               if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> >
> > -               eventdev->data->dev_id = dev_id;
> > -               eventdev->data->socket_id = socket_id;
> > -               eventdev->data->dev_started = 0;
> > +                       strlcpy(eventdev->data->name, name,
> > +                               RTE_EVENTDEV_NAME_MAX_LEN);
> >
> > -               eventdev->attached = RTE_EVENTDEV_ATTACHED;
> > +                       eventdev->data->dev_id = dev_id;
> > +                       eventdev->data->socket_id = socket_id;
> > +                       eventdev->data->dev_started = 0;
> > +               }
> >
> > +               eventdev->attached = RTE_EVENTDEV_ATTACHED;
> >                 eventdev_globals.nb_devs++;
> >         }
> >
> > diff --git a/lib/librte_eventdev/rte_eventdev_pmd_pci.h b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> > index 8fb61386f..443cd38c2 100644
> > --- a/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> > +++ b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> > @@ -112,9 +112,11 @@ rte_event_pmd_pci_remove(struct rte_pci_device *pci_dev,
> >         if (eventdev == NULL)
> >                 return -ENODEV;
> >
> > -       ret = rte_event_dev_close(eventdev->data->dev_id);
> > -       if (ret < 0)
> > -               return ret;
> > +       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +               ret = rte_event_dev_close(eventdev->data->dev_id);
> > +               if (ret < 0)
> > +                       return ret;
> > +       }
> >
> >         /* Invoke PMD device un-init function */
> >         if (devuninit)
> > --
> > 2.17.1
> >
Varghese, Vipin May 2, 2020, 12:07 p.m. UTC | #3
Hi Pavan,

snipped
> > >
> > > When probing event device in secondary process skip reinitializing
> > > the device data structure as it is already done in primary process.
> > >
> > > When removing event device in secondary process skip closing the
> > > event device as it should be done by primary process.
If primary has crashed or closed before secondary abnormally. Should not close of secondary trigger removal of Eventdev services?

> > >
> > > Fixes: 322d0345c2bc ("eventdev: implement PMD registration
> > > functions")
> >
> > Cc: stable@dpdk.org
> >
> > >
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> >
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
> 
> 
> Applied to dpdk-next-eventdev/master. Thanks.
> 
> 
> >
> >
snipped
Pavan Nikhilesh Bhagavatula May 2, 2020, 1:04 p.m. UTC | #4
Hi Vipin,

>Hi Pavan,
>
>snipped
>> > >
>> > > When probing event device in secondary process skip reinitializing
>> > > the device data structure as it is already done in primary process.
>> > >
>> > > When removing event device in secondary process skip closing the
>> > > event device as it should be done by primary process.
>If primary has crashed or closed before secondary abnormally. Should
>not close of secondary trigger removal of Eventdev services?

Closing event device on exit of one secondary doesn’t make sense as
there might be systems where there might be one primary and multiple
secondaries and secondaries are spawned/destroyed on demand.

Behavior of secondaries on primary process crash is undefined.


>
>> > >
>> > > Fixes: 322d0345c2bc ("eventdev: implement PMD registration
>> > > functions")
>> >
>> > Cc: stable@dpdk.org
>> >
>> > >
>> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> >
>> >
>> > Acked-by: Jerin Jacob <jerinj@marvell.com>
>>
>>
>> Applied to dpdk-next-eventdev/master. Thanks.
>>
>>
>> >
>> >
>snipped
Varghese, Vipin May 3, 2020, 1:15 a.m. UTC | #5
Hi Pavan,

Snipped

> >> > >
> >> > > When probing event device in secondary process skip
> >> > > reinitializing the device data structure as it is already done in primary
> process.
> >> > >
> >> > > When removing event device in secondary process skip closing the
> >> > > event device as it should be done by primary process.
> >If primary has crashed or closed before secondary abnormally. Should
> >not close of secondary trigger removal of Eventdev services?
> 
> Closing event device on exit of one secondary doesn’t make sense as there
> might be systems where there might be one primary and multiple secondaries
> and secondaries are spawned/destroyed on demand.
> 
> Behavior of secondaries on primary process crash is undefined.
Absolutely true, there are work scenarios where primary configures ports and Eventdev queues-ports pair. It would be multiple secondaries which process packets via event dev. In such cases, when primary segfaults or crashes it will lead to undefined states.

In such scenarios, would not it be preferer able for all secondaries to subscribe to service function like health check. If the primary is not alive anymore, then gracefully handle inflight events and events to dequeue. If this is right understanding, should not there be option in secondary to gracefully shut down it's worker queue and ports (rather than event device instance)?

snipped
Jerin Jacob May 3, 2020, 9:58 a.m. UTC | #6
On Sun, May 3, 2020 at 6:45 AM Varghese, Vipin <vipin.varghese@intel.com> wrote:
>
> Hi Pavan,
>
> Snipped
>
> > >> > >
> > >> > > When probing event device in secondary process skip
> > >> > > reinitializing the device data structure as it is already done in primary
> > process.
> > >> > >
> > >> > > When removing event device in secondary process skip closing the
> > >> > > event device as it should be done by primary process.
> > >If primary has crashed or closed before secondary abnormally. Should
> > >not close of secondary trigger removal of Eventdev services?
> >
> > Closing event device on exit of one secondary doesn’t make sense as there
> > might be systems where there might be one primary and multiple secondaries
> > and secondaries are spawned/destroyed on demand.
> >
> > Behavior of secondaries on primary process crash is undefined.
> Absolutely true, there are work scenarios where primary configures ports and Eventdev queues-ports pair. It would be multiple secondaries which process packets via event dev. In such cases, when primary segfaults or crashes it will lead to undefined states.
>
> In such scenarios, would not it be preferer able for all secondaries to subscribe to service function like health check. If the primary is not alive anymore, then gracefully handle inflight events and events to dequeue. If this is right understanding, should not there be option in secondary to gracefully shut down it's worker queue and ports (rather than event device instance)?

The health check function may not be limited to eventdev, it must
apply for all the subsystems in multiprocess mode if primary dies.
Such features can be designed/agreed based on every subsystem in mind.
For rc2, Let's have this bug fix. New features can be implemented
after an agreement with all stakeholders wrt multi-process semantics
which applicable for all subsystems.


>
> snipped
Varghese, Vipin May 3, 2020, 12:57 p.m. UTC | #7
Thanks Jerin, agree with you on graceful shutdown in rc2.

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Sunday, May 3, 2020 3:28 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>
> Cc: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for
> secondary process
> 
> On Sun, May 3, 2020 at 6:45 AM Varghese, Vipin <vipin.varghese@intel.com>
> wrote:
> >
> > Hi Pavan,
> >
> > Snipped
> >
> > > >> > >
> > > >> > > When probing event device in secondary process skip
> > > >> > > reinitializing the device data structure as it is already
> > > >> > > done in primary
> > > process.
> > > >> > >
> > > >> > > When removing event device in secondary process skip closing
> > > >> > > the event device as it should be done by primary process.
> > > >If primary has crashed or closed before secondary abnormally.
> > > >Should not close of secondary trigger removal of Eventdev services?
> > >
> > > Closing event device on exit of one secondary doesn’t make sense as
> > > there might be systems where there might be one primary and multiple
> > > secondaries and secondaries are spawned/destroyed on demand.
> > >
> > > Behavior of secondaries on primary process crash is undefined.
> > Absolutely true, there are work scenarios where primary configures ports
> and Eventdev queues-ports pair. It would be multiple secondaries which
> process packets via event dev. In such cases, when primary segfaults or crashes
> it will lead to undefined states.
> >
> > In such scenarios, would not it be preferer able for all secondaries to
> subscribe to service function like health check. If the primary is not alive
> anymore, then gracefully handle inflight events and events to dequeue. If this
> is right understanding, should not there be option in secondary to gracefully
> shut down it's worker queue and ports (rather than event device instance)?
> 
> The health check function may not be limited to eventdev, it must apply for all
> the subsystems in multiprocess mode if primary dies.
> Such features can be designed/agreed based on every subsystem in mind.
> For rc2, Let's have this bug fix. New features can be implemented after an
> agreement with all stakeholders wrt multi-process semantics which applicable
> for all subsystems.
> 
> 
> >
> > snipped

Patch
diff mbox series

diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index b987e0745..9aca7fbd5 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -1364,14 +1364,17 @@  rte_event_pmd_allocate(const char *name, int socket_id)
 
 		eventdev->data = eventdev_data;
 
-		strlcpy(eventdev->data->name, name, RTE_EVENTDEV_NAME_MAX_LEN);
+		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 
-		eventdev->data->dev_id = dev_id;
-		eventdev->data->socket_id = socket_id;
-		eventdev->data->dev_started = 0;
+			strlcpy(eventdev->data->name, name,
+				RTE_EVENTDEV_NAME_MAX_LEN);
 
-		eventdev->attached = RTE_EVENTDEV_ATTACHED;
+			eventdev->data->dev_id = dev_id;
+			eventdev->data->socket_id = socket_id;
+			eventdev->data->dev_started = 0;
+		}
 
+		eventdev->attached = RTE_EVENTDEV_ATTACHED;
 		eventdev_globals.nb_devs++;
 	}
 
diff --git a/lib/librte_eventdev/rte_eventdev_pmd_pci.h b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
index 8fb61386f..443cd38c2 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd_pci.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
@@ -112,9 +112,11 @@  rte_event_pmd_pci_remove(struct rte_pci_device *pci_dev,
 	if (eventdev == NULL)
 		return -ENODEV;
 
-	ret = rte_event_dev_close(eventdev->data->dev_id);
-	if (ret < 0)
-		return ret;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = rte_event_dev_close(eventdev->data->dev_id);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Invoke PMD device un-init function */
 	if (devuninit)