RE: [PATCH v2 03/24] net/nfp: add the flow APIs of nfp PMD

[Chaoyong He <chaoyong.he@corigine.com>]

> On 10/10/2022 7:08 AM, Chaoyong He wrote:
> > Add the flow validate/create/query/destroy/flush API of nfp PMD.
> >
> > The flow create API construct a control cmsg and send it to firmware,
> > then add this flow  to the hash table.
> >
> > The flow query API get flow stats from the flow_priv structure.
> > Note there exist an rte_spin_lock to prevent the update and query
> > action occur at the same time.
> >
> > The flow destroy API construct a control cmsg and send it to firmware,
> > then adelete this flow from the hash table.
> >
> > The flow flush API just iterate the flows in hash table and call the
> > flow destroy API.
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> <...>
> 
> > +static void
> > +nfp_flow_stats_get(struct rte_eth_dev *dev,
> > +             struct rte_flow *nfp_flow,
> > +             void *data)
> > +{
> > +     uint32_t ctx_id;
> > +     struct rte_flow *flow;
> > +     struct nfp_flow_priv *priv;
> > +     struct nfp_fl_stats *stats;
> > +     struct rte_flow_query_count *query;
> > +
> > +     priv = nfp_flow_dev_to_priv(dev);
> > +     flow = nfp_flow_table_search(priv, nfp_flow);
> > +     if (flow == NULL) {
> > +             PMD_DRV_LOG(ERR, "Can not find statistics for this flow.");
> > +             return;
> > +     }
> > +
> > +     query = (struct rte_flow_query_count *)data;
> > +     ctx_id = rte_be_to_cpu_32(nfp_flow->payload.meta->host_ctx_id);
> > +     stats = &priv->stats[ctx_id];
> > +
> > +     rte_spinlock_lock(&priv->stats_lock);
> > +     if (stats->pkts && stats->bytes) {
> 
> Is it guaranteed that 'query' ("void *data") is zeroed out when it is provided
> by application?
> 
> > +             query->hits = stats->pkts;
> > +             query->bytes = stats->bytes;
> > +             query->hits_set = 1;
> > +             query->bytes_set = 1;
> > +             stats->pkts = 0;
> > +             stats->bytes = 0;
> 
> need to check 'reset' field of action to decide reset or not.
> 
> <...>
> 
> > @@ -75,6 +101,7 @@ struct nfp_fl_stats {
> >
> >   struct nfp_flow_priv {
> >       uint32_t hash_seed; /**< Hash seed for hash tables in this
> > structure. */
> > +     uint64_t flower_version; /**< Flow version, always increase. */
> 
> Is this version to keep unique value per flow configuration? If so as far as I
> can see '.validate' is updating this value, is this expected?
> 
> Also who suppose to use this value?

Yes, it is expected.

This value is part of the nfp_flow_meta, and which is part of the flow offloaded
to the firmware.
And the content of the flow offloaded to the firmware is the ABI of the firmware,
so it's can't easily change.