[dpdk-dev,v4,1/2] librte_ether: add internal callback functions
Commit Message
add _rte_eth_dev_callback_process_vf function.
add _rte_eth_dev_callback_process_generic function
Adding a callback to the user application on VF to PF mailbox message,
allows passing information to the application controlling the PF
when a VF mailbox event message is received, such as VF reset.
Signed-off-by: Alex Zelezniak <az5157@att.com>
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
lib/librte_ether/rte_ethdev.c | 17 +++++++++++++
lib/librte_ether/rte_ethdev.h | 44 ++++++++++++++++++++++++++++++++++
lib/librte_ether/rte_ether_version.map | 2 ++
3 files changed, 63 insertions(+)
Comments
2016-10-04 15:52, Bernard Iremonger:
> add _rte_eth_dev_callback_process_vf function.
> add _rte_eth_dev_callback_process_generic function
>
> Adding a callback to the user application on VF to PF mailbox message,
> allows passing information to the application controlling the PF
> when a VF mailbox event message is received, such as VF reset.
I have some difficulties to parse this explanation.
Please could you reword it and precise the direction of the message
and the use case context?
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2510,6 +2510,20 @@ void
> _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> enum rte_eth_event_type event)
> {
> + return _rte_eth_dev_callback_process_generic(dev, event, NULL);
> +}
> +
> +void
> +_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev,
> + enum rte_eth_event_type event, void *param)
> +{
> + return _rte_eth_dev_callback_process_generic(dev, event, param);
> +}
This function is just adding a parameter, compared to the legacy
_rte_eth_dev_callback_process.
Why calling it process_vf?
And by the way, why not just replacing the legacy function?
As it is a driver interface, there is no ABI restriction.
> +
> +void
> +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
> + enum rte_eth_event_type event, void *param)
> +{
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -3026,6 +3026,7 @@ enum rte_eth_event_type {
> /**< queue state event (enabled/disabled) */
> RTE_ETH_EVENT_INTR_RESET,
> /**< reset interrupt event, sent to VF on PF reset */
> + RTE_ETH_EVENT_VF_MBOX, /**< PF mailbox processing callback */
> RTE_ETH_EVENT_MAX /**< max value of this enum */
> };
Either we choose to have a "generic" VF event well documented,
or it is just a specific event with a tip on where to find the doc.
Here we need at least to know how to handle the argument.
> +/**
> + * @internal Executes all the user application registered callbacks. Used by:
> + * _rte_eth_dev_callback_process and _rte_eth_dev_callback_process_vf
> + * It is for DPDK internal user only. User application should not call it
> + * directly.
> + *
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param event
> + * Eth device interrupt event type.
> + *
> + * @param param
> + * parameters to pass back to user application.
> + *
> + * @return
> + * void
> + */
> +void
> +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
> + enum rte_eth_event_type event, void *param);
This is really an internal function and should not be exported at all.
Hi Thomas,
<snip>
> Subject: Re: [dpdk-dev] [PATCH v4 1/2] librte_ether: add internal callback
> functions
>
> 2016-10-04 15:52, Bernard Iremonger:
> > add _rte_eth_dev_callback_process_vf function.
> > add _rte_eth_dev_callback_process_generic function
> >
> > Adding a callback to the user application on VF to PF mailbox message,
> > allows passing information to the application controlling the PF when
> > a VF mailbox event message is received, such as VF reset.
>
> I have some difficulties to parse this explanation.
> Please could you reword it and precise the direction of the message and the
> use case context?
I will reword the explanation and add use case context.
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -2510,6 +2510,20 @@ void
> > _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > enum rte_eth_event_type event)
> > {
> > + return _rte_eth_dev_callback_process_generic(dev, event, NULL); }
> > +
> > +void
> > +_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev,
> > + enum rte_eth_event_type event, void *param) {
> > + return _rte_eth_dev_callback_process_generic(dev, event, param);
> }
>
> This function is just adding a parameter, compared to the legacy
> _rte_eth_dev_callback_process.
> Why calling it process_vf?
The parameter is just being added for the VF event, the handling of the other events is unchanged.
> And by the way, why not just replacing the legacy function?
> As it is a driver interface, there is no ABI restriction.
I thought there would be an ABI issue if the legacy function is replaced.
The _rte_eth_dev_callback_process is exported in DPDK 2.2 and used in the following PMD's, lib and app:
app/test/virtual_pmd
drivers/net/e1000
drivers/net/ixgbe
drivers/net/mlx5
drivers/net/vhost
drivers/net/virtio
lib/librte_ether
Adding a parameter to _rte_eth_dev_callback_process() will impact all of the above.
Will this cause an ABI issue?
> > +
> > +void
> > +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
> > + enum rte_eth_event_type event, void *param) {
> [...]
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -3026,6 +3026,7 @@ enum rte_eth_event_type {
> > /**< queue state event (enabled/disabled)
> */
> > RTE_ETH_EVENT_INTR_RESET,
> > /**< reset interrupt event, sent to VF on PF reset */
> > + RTE_ETH_EVENT_VF_MBOX, /**< PF mailbox processing callback */
> > RTE_ETH_EVENT_MAX /**< max value of this enum */
> > };
>
> Either we choose to have a "generic" VF event well documented, or it is just
> a specific event with a tip on where to find the doc.
> Here we need at least to know how to handle the argument.
It is a specific event for VF to PF messages, details on the function and arguments are in the rte_ethdev.h file.
> > +/**
> > + * @internal Executes all the user application registered callbacks. Used
> by:
> > + * _rte_eth_dev_callback_process and
> _rte_eth_dev_callback_process_vf
> > + * It is for DPDK internal user only. User application should not
> > +call it
> > + * directly.
> > + *
> > + * @param dev
> > + * Pointer to struct rte_eth_dev.
> > + * @param event
> > + * Eth device interrupt event type.
> > + *
> > + * @param param
> > + * parameters to pass back to user application.
> > + *
> > + * @return
> > + * void
> > + */
> > +void
> > +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
> > + enum rte_eth_event_type event, void
> *param);
>
> This is really an internal function and should not be exported at all.
Both new functions are internal I will make them static and remove them from the map file.
When the functions are made static, should the function declarations be moved from rte_ethdev.h to rte_ethdev.c ?
Thanks for the review.
Regards,
Bernard.
2016-10-05 17:04, Iremonger, Bernard:
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -2510,6 +2510,20 @@ void
> > > _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > > enum rte_eth_event_type event)
> > > {
> > > + return _rte_eth_dev_callback_process_generic(dev, event, NULL); }
> > > +
> > > +void
> > > +_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev,
> > > + enum rte_eth_event_type event, void *param) {
> > > + return _rte_eth_dev_callback_process_generic(dev, event, param);
> > }
> >
> > This function is just adding a parameter, compared to the legacy
> > _rte_eth_dev_callback_process.
> > Why calling it process_vf?
>
> The parameter is just being added for the VF event, the handling of the other events is unchanged.
>
> > And by the way, why not just replacing the legacy function?
> > As it is a driver interface, there is no ABI restriction.
>
> I thought there would be an ABI issue if the legacy function is replaced.
> The _rte_eth_dev_callback_process is exported in DPDK 2.2 and used in the following PMD's, lib and app:
>
> app/test/virtual_pmd
> drivers/net/e1000
> drivers/net/ixgbe
> drivers/net/mlx5
> drivers/net/vhost
> drivers/net/virtio
> lib/librte_ether
>
> Adding a parameter to _rte_eth_dev_callback_process() will impact all of the above.
> Will this cause an ABI issue?
No because ABI is for applications (Application Binary Interface).
Here you are just changing the driver interface. And we have no commitment
to maintain the compatibility of this interface for external drivers.
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > @@ -3026,6 +3026,7 @@ enum rte_eth_event_type {
> > > /**< queue state event (enabled/disabled)
> > */
> > > RTE_ETH_EVENT_INTR_RESET,
> > > /**< reset interrupt event, sent to VF on PF reset */
> > > + RTE_ETH_EVENT_VF_MBOX, /**< PF mailbox processing callback */
> > > RTE_ETH_EVENT_MAX /**< max value of this enum */
> > > };
> >
> > Either we choose to have a "generic" VF event well documented, or it is just
> > a specific event with a tip on where to find the doc.
> > Here we need at least to know how to handle the argument.
>
> It is a specific event for VF to PF messages, details on the function and arguments are in the rte_ethdev.h file.
No I think it is only explained in the ixgbe code.
@@ -2510,6 +2510,20 @@ void
_rte_eth_dev_callback_process(struct rte_eth_dev *dev,
enum rte_eth_event_type event)
{
+ return _rte_eth_dev_callback_process_generic(dev, event, NULL);
+}
+
+void
+_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev,
+ enum rte_eth_event_type event, void *param)
+{
+ return _rte_eth_dev_callback_process_generic(dev, event, param);
+}
+
+void
+_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
+ enum rte_eth_event_type event, void *param)
+{
struct rte_eth_dev_callback *cb_lst;
struct rte_eth_dev_callback dev_cb;
@@ -2519,6 +2533,9 @@ _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
continue;
dev_cb = *cb_lst;
cb_lst->active = 1;
+ if (param != NULL)
+ dev_cb.cb_arg = (void *) param;
+
rte_spinlock_unlock(&rte_eth_dev_cb_lock);
dev_cb.cb_fn(dev->data->port_id, dev_cb.event,
dev_cb.cb_arg);
@@ -3026,6 +3026,7 @@ enum rte_eth_event_type {
/**< queue state event (enabled/disabled) */
RTE_ETH_EVENT_INTR_RESET,
/**< reset interrupt event, sent to VF on PF reset */
+ RTE_ETH_EVENT_VF_MBOX, /**< PF mailbox processing callback */
RTE_ETH_EVENT_MAX /**< max value of this enum */
};
@@ -3093,6 +3094,49 @@ void _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
enum rte_eth_event_type event);
/**
+ * @internal Executes all the user application registered callbacks for
+ * the specific device where parameter have to be passed to user application.
+ * It is for DPDK internal user only. User application should not call it
+ * directly.
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param event
+ * Eth device interrupt event type.
+ *
+ * @param param
+ * parameters to pass back to user application.
+ *
+ * @return
+ * void
+ */
+
+void
+_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev,
+ enum rte_eth_event_type event, void *param);
+
+/**
+ * @internal Executes all the user application registered callbacks. Used by:
+ * _rte_eth_dev_callback_process and _rte_eth_dev_callback_process_vf
+ * It is for DPDK internal user only. User application should not call it
+ * directly.
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param event
+ * Eth device interrupt event type.
+ *
+ * @param param
+ * parameters to pass back to user application.
+ *
+ * @return
+ * void
+ */
+void
+_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
+ enum rte_eth_event_type event, void *param);
+
+/**
* When there is no rx packet coming in Rx Queue for a long time, we can
* sleep lcore related to RX Queue for power saving, and enable rx interrupt
* to be triggered when rx packect arrives.
@@ -143,6 +143,8 @@ DPDK_16.07 {
DPDK_16.11 {
global:
+ _rte_eth_dev_callback_process_generic;
+ _rte_eth_dev_callback_process_vf;
rte_eth_dev_pci_probe;
rte_eth_dev_pci_remove;