[ovs-dev] [PATCH 2/2] dpif-netdev: Indicate support for various ct features.

Darrell Ball dlu998 at gmail.com
Wed Jul 19 20:18:47 UTC 2017


On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpettit at ovn.org> wrote:

>
> > On Jul 19, 2017, at 10:45 AM, Darrell Ball <dball at vmware.com> wrote:
> >
> >
> >
> > On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit at ovn.org> wrote:
> >
> >
> >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball at 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 <dlu998 at 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list