On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpet...@ovn.org> wrote:
> > > 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? > [Darrell] I could not see any; it is good the impact from this hybrid support thing was limited to debug o/p then. I don't see any new issues from this patch addition, and it is the only reasonable option for backporting. In your original commit message, you said: "The userspace datapath hardcodes support for the features it supports, but it was missing "ct_state_nat", "ct_orig_tuple", and "ct_orig_tuple6"." Can you modify that so that it reflects that it is partially true and the impact we now know/assume is limited to debug o/p per your confirmation.. Otherwise Acked-by: Darrell Ball <dlu...@gmail.com> //////////////// > > 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. > [Darrell] JTBC, I don't think populating this structure per this patch will cause any additional issues. I was more concerned that if there was any existing issues besides debug o/p, that we might not have full test coverage. But since we don't think that is the case, the additional testing is not needed. //////////////// > > >> 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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev