> On Jul 19, 2017, at 10:45 AM, Darrell Ball <db...@vmware.com> wrote:
>
>
>
> On 7/19/17, 8:01 AM, "Justin Pettit" <jpet...@ovn.org> wrote:
>
>
>> On Jul 19, 2017, at 2:45 AM, Darrell Ball <db...@vmware.com> wrote:
>>
>> This change does not seem to be all that useful.
>>
>> When rules are constructed, mask and action support do check previously
>> probed support
>> which will be ‘TRUE’.
>>
>> Another way to see that the below settings are not useful is to set
>> everything to ‘false’ (see below) and run all
>> the system tests in userspace, which will all pass.
>
> This change does affect behavior. For example, upcall debug log messages
> will now show ct_orig_tuple members. The reason
> is that dp_netdev_upcall() passes those support parameters to
> odp_flow_key_from_flow(), which thinks that the datapath
> doesn't support those features, so it doesn't print the values.
>
> I see, so you noticed it affected debug output.
> I see the hardcoded values passed in 3 functions.
> Was there any other impact you can foresee ?
I don't. Do you?
> Any additional test coverage you can recommend ?
I don't think so, since likely future issues will come from adding new support
features and not populating this structure--not from this reverting somehow.
>> By the way, the .max_vlan_headers and .max_mpls_headers fields, which I did
>> not change are pretty big
>> numbers and I am fairly sure OVS does not really support that many vlans and
>> labels.
>>
>> static struct odp_support dp_netdev_support = {
>> .max_vlan_headers = SIZE_MAX,
>> .max_mpls_depth = SIZE_MAX,
>> .recirc = false,
>> .ct_state = false,
>> .ct_zone = false,
>> .ct_mark = false,
>> .ct_label = false,
>> .ct_state_nat = false,
>> .ct_orig_tuple = false,
>> .ct_orig_tuple6 = false,
>> };
>>
>> I think it may be better to clean this up. I can do this if you are ok with
>> that; either way is fine with me.
>
> I agree that since the datapath features are probed, we should pass those
> parameters around instead of using these hardcoded values. However, it was a
> more invasive change than I wanted to make at the time, and I'd want to apply
> this fix regardless. I was going to add using the probed values to my to-do
> list, but I'm happy if you want to take it.
>
> If there is a pressing reason to make a temporary fix, then that is fine.
This seems like an obvious fix, and can be safely backported to earlier
version. I agree that feature-probing is a better long-term solution, but I'd
prefer to backport this rather than a bigger change. I would have proposed
this patch anyway.
By the way, can you look at adjusting your email client to quote replies
instead of merely indenting them? It makes it hard to see what the other
person was saying when you reply.
--Justin
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
