| Message ID | 1475592734-22355-2-git-send-email-bernard.iremonger@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
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.
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index c517e88..e850d88 100644 --- 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); +} + +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); diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index 7218b6f..b418c5e 100644 --- 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 */ }; @@ -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. diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map index 72be66d..6f97a8f 100644 --- a/lib/librte_ether/rte_ether_version.map +++ b/lib/librte_ether/rte_ether_version.map @@ -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;