On 8/25/2017 2:26 PM, Thomas Monjalon wrote:

24/08/2017 09:12, Shahaf Shuler:

quoted

Thursday, August 24, 2017 1:06 AM, Thomas Monjalon:

quoted

23/08/2017 15:13, Shahaf Shuler:

quoted

Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:

quoted

From: Shahaf Shuler

quoted

In order to enable PMDs to support only one of the APIs, and
applications to avoid branching according to the underlying device
a copy functions to/from the old/new APIs were added.

Looks a good intent.
I would prefer the word "convert" instead of "copy".

quoted

quoted

quoted

 int
 rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,

[...]

quoted

quoted

quoted

+	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
+		   (dev->data->dev_conf.rxmode.ignore == 1)) {
+		int ret;
+		struct rte_eth_rxmode rxmode;
+
+		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
+		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
+			   sizeof(rxmode))) {
+			/*
+			 * device which work with rxmode offloads API requires
+			 * a re-configuration in order to apply the new offloads
+			 * configuration.
+			 */
+			dev->data->dev_conf.rxmode = rxmode;
+			ret = rte_eth_dev_configure(port_id,
+					dev->data->nb_rx_queues,
+					dev->data->nb_tx_queues,
+					&dev->data->dev_conf);

Hmm, and why we would need to reconfigure our device in the middle
of rx queue setup?

The reason is the old Rx offloads API is configured on device configure.
This if section is for applications which already moved to the new
offload API however the underlying PMD still uses the old one.

Isn't it risky to re-run configure here?
We could also declare this case as an error.

I think applications which have migrated to the new API, could use the
convert functions themselves before calling configure to support not
migrated PMDs.
The cons of my solution are:
- discourage apps to migrate before all PMDs have migrated
- expose a temporary function to convert API I propose it anyway because
there is always someone to like bad ideas ;)

Yes. I tried to make it as simple as possible for application to move to the new API.
Defining it as error flow, will enforce the application to check the PMD offload mode and branch accordingly. The conversion functions are a good helpers, yet the code remains complex due to the different cases with the different PMDs.

Considering the re-configuration is risky, and without other ideas I will need to fall back to the error flow case.
Are we OK with that?

I think we can take the risk of keeping this call to
rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
In theory it should be acceptable.
If we merge it soon, it can be better tested with every drivers.

I doubt about taking that risk. Some driver does HW configuration via
configure() and combination of start/stop, setup_queue and configure can
be complex.

I am for generating error for this case.

Generating error also can be good motivation for PMDs to adapt new method.