Hi Thomasz,

quoted

2016-10-27 16:24, Ananyev, Konstantin:

quoted

From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]

quoted

2016-10-27 15:52, Ananyev, Konstantin:

quoted

quoted

Hi Tomasz,

This is a major new function in the API and I still have some comments.

2016-10-26 14:56, Tomasz Kulasek:

quoted

--- a/config/common_base
+++ b/config/common_base
+CONFIG_RTE_ETHDEV_TX_PREP=y

We cannot enable it until it is implemented in every drivers.

Not sure why?
If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
Right now it is not mandatory for the PMD to implement it.

If it is not implemented, the application must do the preparation by itself.
From patch 6:
"
Removed pseudo header calculation for udp/tcp/tso packets from
application and used Tx preparation API for packet preparation and
verification.
"
So how does it behave with other drivers?

Hmm so it seems that we broke testpmd csumonly mode for non-intel drivers..
My bad, missed that part completely.
Yes, then I suppose for now we'll need to support both (with and without) code paths for testpmd.
Probably a new fwd mode or just extra parameter for the existing one?
Any other suggestions?

Please think how we can use it in every applications.
It is not ready.
Either we introduce the API without enabling it, or we implement it
in every drivers.

I understand your position here, but just like to point that:
1) It is a new functionality optional to use.
     The app is free not to use that functionality and still do the preparation itself
     (as it has to do it now).
    All existing apps would keep working as expected without using that function.
    Though if the app developer knows that for all HW models he plans to run on
    tx_prep is implemented - he is free to use it.
    2) It would be difficult for Tomasz (and other Intel guys) to implement tx_prep()
     for all non-Intel HW that DPDK supports right now.
     We just don't have all the actual HW in stock and probably adequate knowledge of it.
    So we depend here on the good will of other PMD mainaners/developers to implement
    tx_prep() for these devices.
    From other side, if it will be disabled by default, then, I think,
    PMD developers just wouldn't be motivated to implement it.
    So it will be left untested and unused forever.

Actually as another thought:
Can we have it enabled by default, but mark it as experimental or so?
If memory serves me right, we've done that for cryptodev in the past, no?
Konstantin

quoted

quoted

quoted

quoted

quoted

quoted

 struct rte_eth_dev {
 	eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
 	eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
+	eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare function. */
 	struct rte_eth_dev_data *data;  /**< Pointer to device data */
 	const struct eth_driver *driver;/**< Driver for this device */
 	const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */

Could you confirm why tx_pkt_prep is not in dev_ops?
I guess we want to have several implementations?

Yes, it depends on configuration options, same as tx_pkt_burst.

quoted

Shouldn't we have a const struct control_dev_ops and a struct datapath_dev_ops?

That's probably a good idea, but I suppose it is out of scope for that patch.

No it's not out of scope.
It answers to the question "why is it added in this structure and not dev_ops".
We won't do this change when nothing else is changed in the struct.

Not sure I understood you here:
Are you saying datapath_dev_ops/controlpath_dev_ops have to be introduced as part of that patch?
But that's a lot of  changes all over rte_ethdev.[h,c].
It definitely worse a separate patch (might be some discussion) for me.

Yes it could be a separate patch in the same patchset.

Honestly, I think it is a good idea, but it is too late and too risky to do such change right now.
We are on RC2 right now, just few days before RC3...
Can't that wait till 17.02?
From my understanding - it is pure code restructuring, without any functionality affected.
Konstantin