eventdev/eth_rx: fix timestamp field register in mbuf
Checks
Commit Message
For eventdev internal port, timestamp dynamic field registration
in mbuf is not required as that will be done from net device.
For SW eventdev, Rx timestamp field registration will be
done during Rx queue add operation as per device capabilities and
offload configuration.
Fixes: 83ab470d1259 ("eventdev/eth_rx: use timestamp as dynamic mbuf field")
Cc: stable@dpdk.org
Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
---
lib/eventdev/rte_event_eth_rx_adapter.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
Comments
On Mon, Sep 18, 2023 at 6:46 PM Rahul Bhansali <rbhansali@marvell.com> wrote:
>
> For eventdev internal port, timestamp dynamic field registration
> in mbuf is not required as that will be done from net device.
> For SW eventdev, Rx timestamp field registration will be
> done during Rx queue add operation as per device capabilities and
> offload configuration.
>
> Fixes: 83ab470d1259 ("eventdev/eth_rx: use timestamp as dynamic mbuf field")
> Cc: stable@dpdk.org
>
> Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
Ping for review from maintainer.
> -----Original Message-----
> From: Rahul Bhansali <rbhansali@marvell.com>
> Sent: Monday, September 18, 2023 1:56 PM
> To: dev@dpdk.org; Naga Harish K, S V <s.v.naga.harish.k@intel.com>; Jerin
> Jacob <jerinj@marvell.com>; Kundapura, Ganapati
> <ganapati.kundapura@intel.com>
> Cc: Rahul Bhansali <rbhansali@marvell.com>; stable@dpdk.org
> Subject: [PATCH] eventdev/eth_rx: fix timestamp field register in mbuf
>
> For eventdev internal port, timestamp dynamic field registration in mbuf is not
> required as that will be done from net device.
> For SW eventdev, Rx timestamp field registration will be done during Rx queue
> add operation as per device capabilities and offload configuration.
>
> Fixes: 83ab470d1259 ("eventdev/eth_rx: use timestamp as dynamic mbuf
> field")
> Cc: stable@dpdk.org
>
> Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> ---
> lib/eventdev/rte_event_eth_rx_adapter.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c
> b/lib/eventdev/rte_event_eth_rx_adapter.c
> index 3ebfa5366d..5a5fade466 100644
> --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> @@ -2472,13 +2472,6 @@ rxa_create(uint8_t id, uint8_t dev_id,
> if (conf_cb == rxa_default_conf_cb)
> rx_adapter->default_cb_arg = 1;
>
> - if (rte_mbuf_dyn_rx_timestamp_register(
> - &event_eth_rx_timestamp_dynfield_offset,
> - &event_eth_rx_timestamp_dynflag) != 0) {
> - RTE_EDEV_LOG_ERR("Error registering timestamp field in
> mbuf\n");
> - return -rte_errno;
> - }
> -
> rte_eventdev_trace_eth_rx_adapter_create(id, dev_id, conf_cb,
> conf_arg);
> return 0;
> @@ -2738,6 +2731,7 @@ rte_event_eth_rx_adapter_queue_add(uint8_t id,
> 1);
> }
> } else {
> + uint64_t dev_offloads;
> rte_spinlock_lock(&rx_adapter->rx_lock);
> dev_info->internal_event_port = 0;
> ret = rxa_init_service(rx_adapter, id); @@ -2749,6 +2743,17
> @@ rte_event_eth_rx_adapter_queue_add(uint8_t id,
> rxa_sw_adapter_queue_count(rx_adapter));
> }
> rte_spinlock_unlock(&rx_adapter->rx_lock);
> +
> + dev_offloads = dev_info->dev->data-
> >dev_conf.rxmode.offloads;
This is a one-time operation and need not happen for every queue_add.
Move this registration to "rxa_init_service()" function which executes only once for creating rte_service.
Also, no need to check for offload capabilities and directly do the registration inside
Rxa_init_service as done before in rxa_create.
Mbuf field is global to the entire application and need not be done based on ethdev offload capabilities.
> + if (dev_offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> + if (rte_mbuf_dyn_rx_timestamp_register(
> +
> &event_eth_rx_timestamp_dynfield_offset,
> +
> &event_eth_rx_timestamp_dynflag) != 0) {
> + RTE_EDEV_LOG_ERR("Error registering
> timestamp field in mbuf\n");
> + return -rte_errno;
> + }
> + }
> +
> }
>
> rte_eventdev_trace_eth_rx_adapter_queue_add(id, eth_dev_id,
> --
> 2.25.1
> -----Original Message-----
> From: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> Sent: Wednesday, September 20, 2023 6:03 PM
> To: Rahul Bhansali <rbhansali@marvell.com>; dev@dpdk.org; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Kundapura, Ganapati
> <ganapati.kundapura@intel.com>
> Cc: stable@dpdk.org
> Subject: [EXT] RE: [PATCH] eventdev/eth_rx: fix timestamp field register in mbuf
>
> External Email
>
> ----------------------------------------------------------------------
>
>
> > -----Original Message-----
> > From: Rahul Bhansali <rbhansali@marvell.com>
> > Sent: Monday, September 18, 2023 1:56 PM
> > To: dev@dpdk.org; Naga Harish K, S V <s.v.naga.harish.k@intel.com>;
> > Jerin Jacob <jerinj@marvell.com>; Kundapura, Ganapati
> > <ganapati.kundapura@intel.com>
> > Cc: Rahul Bhansali <rbhansali@marvell.com>; stable@dpdk.org
> > Subject: [PATCH] eventdev/eth_rx: fix timestamp field register in mbuf
> >
> > For eventdev internal port, timestamp dynamic field registration in
> > mbuf is not required as that will be done from net device.
> > For SW eventdev, Rx timestamp field registration will be done during
> > Rx queue add operation as per device capabilities and offload configuration.
> >
> > Fixes: 83ab470d1259 ("eventdev/eth_rx: use timestamp as dynamic mbuf
> > field")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> > ---
> > lib/eventdev/rte_event_eth_rx_adapter.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c
> > b/lib/eventdev/rte_event_eth_rx_adapter.c
> > index 3ebfa5366d..5a5fade466 100644
> > --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> > +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> > @@ -2472,13 +2472,6 @@ rxa_create(uint8_t id, uint8_t dev_id,
> > if (conf_cb == rxa_default_conf_cb)
> > rx_adapter->default_cb_arg = 1;
> >
> > - if (rte_mbuf_dyn_rx_timestamp_register(
> > - &event_eth_rx_timestamp_dynfield_offset,
> > - &event_eth_rx_timestamp_dynflag) != 0) {
> > - RTE_EDEV_LOG_ERR("Error registering timestamp field in
> > mbuf\n");
> > - return -rte_errno;
> > - }
> > -
> > rte_eventdev_trace_eth_rx_adapter_create(id, dev_id, conf_cb,
> > conf_arg);
> > return 0;
> > @@ -2738,6 +2731,7 @@ rte_event_eth_rx_adapter_queue_add(uint8_t id,
> > 1);
> > }
> > } else {
> > + uint64_t dev_offloads;
> > rte_spinlock_lock(&rx_adapter->rx_lock);
> > dev_info->internal_event_port = 0;
> > ret = rxa_init_service(rx_adapter, id); @@ -2749,6 +2743,17
> @@
> > rte_event_eth_rx_adapter_queue_add(uint8_t id,
> > rxa_sw_adapter_queue_count(rx_adapter));
> > }
> > rte_spinlock_unlock(&rx_adapter->rx_lock);
> > +
> > + dev_offloads = dev_info->dev->data-
> > >dev_conf.rxmode.offloads;
>
> This is a one-time operation and need not happen for every queue_add.
> Move this registration to "rxa_init_service()" function which executes only once
> for creating rte_service.
>
> Also, no need to check for offload capabilities and directly do the registration
> inside Rxa_init_service as done before in rxa_create.
> Mbuf field is global to the entire application and need not be done based on
> ethdev offload capabilities.
>
>
Ack, will address and send v2.
> > + if (dev_offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> > + if (rte_mbuf_dyn_rx_timestamp_register(
> > +
> > &event_eth_rx_timestamp_dynfield_offset,
> > +
> > &event_eth_rx_timestamp_dynflag) != 0) {
> > + RTE_EDEV_LOG_ERR("Error registering
> > timestamp field in mbuf\n");
> > + return -rte_errno;
> > + }
> > + }
> > +
> > }
> >
> > rte_eventdev_trace_eth_rx_adapter_queue_add(id, eth_dev_id,
> > --
> > 2.25.1
As per the DPDK contribution guidelines,
Ideally, the review should be done within 1 week of patch submission to the mailing list.
https://doc.dpdk.org/guides/contributing/patches.html
Surprised to see this review request ping message the very next day of the patch.
Better to follow the contribution guidelines.
-Harish
________________________________
From: Jerin Jacob <jerinjacobk@gmail.com>
Sent: Tuesday, September 19, 2023 9:59 PM
To: Rahul Bhansali <rbhansali@marvell.com>
Cc: dev@dpdk.org <dev@dpdk.org>; Naga Harish K, S V <s.v.naga.harish.k@intel.com>; Jerin Jacob <jerinj@marvell.com>; Kundapura, Ganapati <Ganapati.Kundapura@intel.com>; stable@dpdk.org <stable@dpdk.org>
Subject: Re: [PATCH] eventdev/eth_rx: fix timestamp field register in mbuf
On Mon, Sep 18, 2023 at 6:46 PM Rahul Bhansali <rbhansali@marvell.com> wrote:
>
> For eventdev internal port, timestamp dynamic field registration
> in mbuf is not required as that will be done from net device.
> For SW eventdev, Rx timestamp field registration will be
> done during Rx queue add operation as per device capabilities and
> offload configuration.
>
> Fixes: 83ab470d1259 ("eventdev/eth_rx: use timestamp as dynamic mbuf field")
> Cc: stable@dpdk.org
>
> Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
Ping for review from maintainer.
On Mon, Sep 25, 2023 at 3:48 PM Naga Harish K, S V
<s.v.naga.harish.k@intel.com> wrote:
>
> As per the DPDK contribution guidelines,
> Ideally, the review should be done within 1 week of patch submission to the mailing list.
> https://doc.dpdk.org/guides/contributing/patches.html
>
> Surprised to see this review request ping message the very next day of the patch.
> Better to follow the contribution guidelines.
+ @Thomas Monjalon @Ali Alnubani @Ferruh Yigit @David Marchand
@Andrew Rybchenko @Ajit Khaparde @Qi Zhang @Raslan Darawsheh @Maxime
Coquelin @cheng1.jiang@intel.com @Akhil Goyal @Richardson, Bruce
Yes, and I agree, and thanks for reporting this. I usually merge a set
of patches together and call for review action if something pending in
my list. I was not looking to the timestamp as it was manual.
Seems like good feature additions in patchwork if it not ready
available or if available then we can configure for DPDK.
i.e., if patchwork can track email time for the patch and flag for no
reviewed happened in N days(N as configurable) to help maintainers.
Also, Get list of pending patches on component maintainers(I think,
currently we can get only based on tree delegation) not per component
maintainer.
>
> -Harish
@@ -2472,13 +2472,6 @@ rxa_create(uint8_t id, uint8_t dev_id,
if (conf_cb == rxa_default_conf_cb)
rx_adapter->default_cb_arg = 1;
- if (rte_mbuf_dyn_rx_timestamp_register(
- &event_eth_rx_timestamp_dynfield_offset,
- &event_eth_rx_timestamp_dynflag) != 0) {
- RTE_EDEV_LOG_ERR("Error registering timestamp field in mbuf\n");
- return -rte_errno;
- }
-
rte_eventdev_trace_eth_rx_adapter_create(id, dev_id, conf_cb,
conf_arg);
return 0;
@@ -2738,6 +2731,7 @@ rte_event_eth_rx_adapter_queue_add(uint8_t id,
1);
}
} else {
+ uint64_t dev_offloads;
rte_spinlock_lock(&rx_adapter->rx_lock);
dev_info->internal_event_port = 0;
ret = rxa_init_service(rx_adapter, id);
@@ -2749,6 +2743,17 @@ rte_event_eth_rx_adapter_queue_add(uint8_t id,
rxa_sw_adapter_queue_count(rx_adapter));
}
rte_spinlock_unlock(&rx_adapter->rx_lock);
+
+ dev_offloads = dev_info->dev->data->dev_conf.rxmode.offloads;
+ if (dev_offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
+ if (rte_mbuf_dyn_rx_timestamp_register(
+ &event_eth_rx_timestamp_dynfield_offset,
+ &event_eth_rx_timestamp_dynflag) != 0) {
+ RTE_EDEV_LOG_ERR("Error registering timestamp field in mbuf\n");
+ return -rte_errno;
+ }
+ }
+
}
rte_eventdev_trace_eth_rx_adapter_queue_add(id, eth_dev_id,