[dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[pbhagavatula]

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(-)

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

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Jerin Jacob]

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
>

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Jerin Jacob]

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
> >

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Varghese, 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?

> > >
> > > 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

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Pavan Nikhilesh Bhagavatula]

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

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Varghese, 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.
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

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Jerin Jacob]

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

Re: [dpdk-dev] [PATCH] eventdev: fix device probe and remove for secondary process

[Varghese, Vipin]

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