On 16.12.2019 05:07, Anju Thomas wrote:
> Thanks for the review Ilya,
>
> For the below comment
>
>
>> @@ -7086,10 +7105,16 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>> * the ownership of these packets. Thus, we can avoid performing
>> * the action, because the caller will not use the result
>> anyway.
>> * Just break to free the batch. */
>> + COVERAGE_ADD(datapath_drop_tunnel_push_error,
>> + dp_packet_batch_size(packets_));
> This is not a tunnel push error. We literally executed all the actions
> without errors and that is not our fault that there are no more actions after
> tnl_push. We're just saving some time avoiding actual execution of a
> pointless action. So, this should not be accounted as error, and definitely
> not as a tunnel push error.
>
> I agree we need a better name .Does datapath_drop_incomplete_dp_action sound
> good or do you have any other suggestions in mind?
datapath_drop_last_action_tnl_push ?
And we actually could just avoid counting in this case because:
1. It's a normal scenario of action execution. If the code looked like this:
case OVS_ACTION_ATTR_TUNNEL_PUSH:
dp_packet_batch_apply_cutlen(packets_);
push_tnl_action(pmd, a, packets_)
break;
you most probably wouldn't noticed this case because it's perfectly normal.
2. We're not tracking similar cases for all other actions. What if push_vlan,
set_ipv4_src or meter is the last action?
The only reason why we have a spacial case with big comment for tunnel push
is that we had dp_packet leak here and we don't really want to execute heavy
push action if we're going to drop these packets anyway.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
