On 12-Jan-21 8:32 PM, Lance Richardson wrote:

quoted

On Thu, Dec 17, 2020 at 9:08 AM Anatoly Burakov
[off-list ref] wrote:

quoted

From: Liang Ma <redacted>

Add a simple API to allow getting the monitor conditions for
power-optimized monitoring of the RX queues from the PMD, as well as
release notes information.

Signed-off-by: Liang Ma <redacted>
Signed-off-by: Anatoly Burakov <redacted>
Acked-by: Konstantin Ananyev <redacted>
---

<snip>

quoted

  /**
   * @internal A structure containing the functions exported by an Ethernet driver.
   */

@@ -917,6 +937,8 @@ struct eth_dev_ops {
         /**< Set up the connection between the pair of hairpin queues. */
         eth_hairpin_queue_peer_unbind_t hairpin_queue_peer_unbind;
         /**< Disconnect the hairpin queues of a pair from each other. */
+       eth_get_monitor_addr_t get_monitor_addr;
+       /**< Get next RX queue ring entry address. */
  };

The implementation of get_monitor_addr will have much in common with
the rx_descriptor_status API in struct rte_eth_dev, including the property
that it will likely not make sense for it to be called concurrently with
rx_pkt_burst on a given queue. Might it make more sense to have this
API in struct rte_eth_dev instead of struct eth_dev_ops?

I don't have an opinion on this as this code isn't really my area of
expertise. I'm fine with wherever the community thinks this code should
be. Any other opinions?

I don't think it is a good idea to  push new members into rte_eth_dev.
It either means an ABI breakage or wasting of one of our reserved fields.
IMO this function is not that performance critical to justify such insersion.
In fact, I think we should look in different direction -
remove rx/tx_descriptor_status() functions from rte_eth_dev,
or even better make rte_eth_dev an opaque pointer.