On 10/19/2022 12:30 PM, Chaoyong He wrote:
On 10/19/2022 4:00 AM, Chaoyong He wrote:
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...@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderl...@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?
Let me clarify this comment,
When "stats->pkts == 0", not taken this branch and 'query' fields are not
updated. How caller can know if 'query' has random values or assigned values,
won't it be good to memset query.
Okay, I understand it now. I'll revise like what you said in the next version
patch, thanks.
+ 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.
And this one also seems not answered, to there is an attribute of action to
request resetting stats, above code ignores it.
How about like this:
```
if (stats->pkts != 0 && stats->bytes != 0) {
query->hits = stats->pkts;
query->bytes = stats->bytes;
query->hits_set = 1;
query->bytes_set = 1;
if (query->reset != 0) {
rte_spinlock_lock(&priv->stats_lock);
stats->pkts = 0;
stats->bytes = 0;
rte_spinlock_unlock(&priv->stats_lock);
}
ack
} else {
memset(query, 0, sizeof(*query));
}
memset can go outside of the if block for simplicity, but both are OK,
up to you.
```
And I'm not quite sure if I should add a check about the `reserved` field like
this:
```
query = (struct rte_flow_query_count *)data;
if (query->reserved != 0) {
PMD_DRV_LOG(ERR, "The reserved field should always be 0!");
return;
}
```
I don't think it is required to check reserved fields.
<...>
@@ -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.
I am not sure we are on same page. This variable is increased when a flow
rule is added or validated by application, how this is part of ABI?
Also whenever application validates a flow this 'flower_version' value is
increased. Application is just validating the flow, not adding a flow so nothing
changes in the HW config.
And this increase in the 'flower_version' variable may cause driver/hw think
that config is changed?
Okay, I will revise to make sure the '.validate' won't change this value
anymore, thanks.
ack
