On Thu, Dec 13, 2012 at 2:08 AM, Jarno Rajahalme <jarno.rajaha...@nsn.com> wrote: > On Dec 12, 2012, at 18:23 , ext Jesse Gross wrote: >> On Wed, Dec 12, 2012 at 6:34 AM, Jarno Rajahalme >> <jarno.rajaha...@nsn.com> wrote: >>> This version adds odp-level support for tunnel metadata. >>> >>> Currently it seems the datapath expects userspace to specify all the >>> tunnel fields, or none of them. IMO, it would be better to use of >>> configured values as defaults, allowing flow entries to provide only >>> some of the fields (e.g., tun_dst). >>> >>> This is obviously similar to how any other field changes are viewed >>> at the OF level, but tunnel metadata is different since packets don't >>> have the metadata when they arrive on a normal port, but the tunnel >>> metadata is figured out only when the output port is determined >>> (which may be taken from a register if I'm not mistaken). >>> >>> So, is the design intent that the userspace code digs the missing >>> values from the tunnel configuration data? Currently, it seems that >>> data is readily available in the datapath when it is needed, but maybe >>> doing it "once" on the userspace is better than figuring out which >>> values to use on the datapath for each packet separately? >> >> The configuration information is no longer going to be available at >> the kernel level soon. This will allow userspace to be more flexible >> about the policies that it implements. >> >> Please separate out the ODP changes from the OpenFlow changes. I >> can't apply the OpenFlow portions until all of the issues that I >> mentioned before have been resolved but I could apply the ODP >> component now. >> > > OK. > >> Is the ODP code based on the patch that I linked to before? If so, it >> makes some changes that I don't understand, particularly things like >> dropping comments and unit tests. >> > > I hadn't seen your patch, sorry. I was just trying to fill the gaps between > the datapath and the Openflow-level patches I sent earlier. > > You had used memcmp() on struct flow_tnl. Currently that would work fine, > given that flow_tnl seems to have no need for compiler generated padding. > It might be better still to compare the individual fields to be more explicit > and future/comiler proof?
I don't think it's necessary because flows should be zeroed out during initialization since they tend to get hashed and compared. >> In any case, flags should be converted between userspace and kernel. >> The same set of flags happen to be defined now with the same values >> but it shouldn't be assumed that is the case. In particular, if there >> are different versions of userspace and kernel then new flags that the >> other side doesn't know about shouldn't just get carried over. This >> can result in not being able to setup flows properly. > > This should probably be marked with BUILD_ASSERT_DECL(FLOW_WC_SEQ == 18) > to remind about updating the flags mapping when they change? The FLOW_WC_SEQ is primarily about things related to the classifier. I'm not sure whether we'll want to directly allow people to match on the flags so I don't know that we need to add that now. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
