Short term solution: Yes, returning "ODP_FIT_TOO_LITTLE" from check_expectations() isn't enough to prevent a flow from being installed in the kernel datapath. OVS userspace, since the introduction of megaflows, handles compatibility with older kernels in a different way: the kernel will refuse to install a flow with a mask that contains unknown attributes.
You can rely on that by: * Adding an attribute for your new field in odp-netlink.h (besides, odp-netlink.h is generated from datapath/linux/compat/include/linux/openvswitch.h, so it's better to change that) * Making sure that your attribute is only interpreted by userspace and rejected by kernel by wrapping it in #ifndef __KERNEL__ * Modifying odp_flow_key_from_flow, odp_flow_key_from_mask, odp_flow_key_to_flow* and odp_flow_key_to_mask* to handle your attribute. When the userspace will try to install a megaflow with a mask on the new attribute the kernel will discard it. Long term solution: We should probably be smarter in userspace and avoid installing a megaflow with a key that returns ODP_FIT_TOO_LITTLE, or (better) avoid installing a megaflow with a mask that can't be supported by the kernel. This is the only way to support matching on a field which doesn't have a corresponding ODP attribute. On 27/04/2016 06:18, "Ben Pfaff" <b...@ovn.org> wrote: >I'm not aware of bugs here, but there could easily be something new, >since this code path isn't exercised very much lately. > >Daniele, did you find anything out? > >On Wed, Apr 27, 2016 at 10:13:58AM +0300, Enas Ahmad wrote: >> I also found a discussion here referring to the same issue, however I am >> not sure if it was solved or not ? >> >> https://patchwork.ozlabs.org/patch/555337/ >> >> >> "....However, slowpathing ODP_FIT_TOO_LITTLE flow keys needs to be added in >> the case where the fields not understood by the datapath are used in the >> flow translation. Daniele promised to take a stab at it." >> >> >> >> On Sun, Apr 24, 2016 at 4:07 PM, Enas Ahmad <enas.ah...@kaust.edu.sa> wrote: >> >> > According to Open vSwitch datapath developer documentation: >> > https://www.kernel.org/doc/Documentation/networking/openvswitch.txt >> > >> > "If the userspace flow key includes more fields than the >> > kernel's, for example if userspace decoded an IPv6 header but >> > the kernel stopped at the Ethernet type, then userspace can >> > forward the packet manually, without setting up a flow in the >> > kernel. This case is bad for performance because every packet >> > that the kernel considers part of the flow must go to userspace, >> > but the forwarding behavior is correct. (If userspace can >> > determine that the values of the extra fields would not affect >> > forwarding behavior, then it could set up a flow anyway.)" >> > >> > However, my new "userspace-only" field is still having flow entries >> > created at the kernel (of course with ignoring my field) which causes >> > inconsistent behavior during the OFPROTO_MAX_IDLE_DEFAULT time until the >> > kernel flow entry expires and the packet is rerouted to the userspace >> > again. I am not planning at this stage to implement my field at the kernel >> > level, therefore I would expect, with a performance cost, that any packet >> > which has my field present to be always routed to the user space for flow >> > matching, correct? >> > >> > I added a key entry for my field in odp-util.c and odp-netlink.h following >> > the existing pattern, which causes the check_expectations() method to >> > return "ODP_FIT_TOO_LITTLE" due to "expected but not present" attribute, >> > which makes perfect sense. However, this was not taken any further and the >> > flow entry is still cached at the kernel. >> > >> > Is there any additional logic I need to add ? >> > >> > >> > >> > On Tue, Mar 22, 2016 at 6:39 PM, Ben Pfaff <b...@ovn.org> wrote: >> > >> >> Thanks, I've sent out a patch for the FAQ. >> >> >> >> On Tue, Mar 22, 2016 at 11:09:54AM +0300, Enas Ahmad wrote: >> >> > I am posting here so if someone else is trying to add a new field to ovs >> >> > maybe this can help and save their time. >> >> > After debugging I solved my issue by adding the following to >> >> nx_put_raw() >> >> > in nx-match.c: >> >> > >> >> > if( match->wc.masks.iot_addr){ >> >> > nxm_put_32m(b, MFF_IOT_ADDR, oxm, >> >> > htonl(flow->iot_addr), match->wc.masks.iot_addr); >> >> > } >> >> > >> >> > otherwise my field was not added to ofpbuf buffer. If correct maybe it >> >> can >> >> > be added to the FAQ. >> >> > >> >> > Best, >> >> > Enas >> >> > >> >> > On Fri, Mar 18, 2016 at 6:50 AM, Ben Pfaff <b...@ovn.org> wrote: >> >> > >> >> > > I'm always frankly puzzled by this kind of message that says "I made >> >> > > some changes to OVS and they didn't work." The answer is that you >> >> need >> >> > > to debug your changes. >> >> > > >> >> > > On Thu, Mar 17, 2016 at 06:17:10PM +0300, Enas Ahmad wrote: >> >> > > > Hi, >> >> > > > I am still having a problem in successfully defining a new field. My >> >> > > field >> >> > > > name is iot_addr. I define it in the meta-flow.h file as the >> >> following: >> >> > > > >> >> > > > /* "iot_addr". >> >> > > > * >> >> > > > * IoT address >> >> > > > * >> >> > > > * Type: be32. >> >> > > > * Maskable: bitwise. >> >> > > > * Formatting: decimal. >> >> > > > * Prerequisites: UDP. >> >> > > > * Access: read/write. >> >> > > > * NXM: none. >> >> > > > * OXM: NXOXM_ET_IOT_ADDR(1) since OF1.5 and v2.5. >> >> > > > */ >> >> > > > MFF_IOT_ADDR, >> >> > > > >> >> > > > When I try to create a new flow entry using ovs-ofctl: >> >> > > > >> >> > > > sudo ovs-ofctl add-flow s1 >> >> > > > >> >> > > >> >> priority=65534,dl_type=0x0800,nw_proto=17,tp_dst=9999,iot_addr=1234567890,actions=output:2 >> >> > > > >> >> > > > the iot_addr is correctly set in the flow-mod msg. >> >> > > > >> >> > > > I also debugged the miniflow_extract() and any incoming packet is >> >> being >> >> > > > correctly parsed and the value is set in iot_addr. >> >> > > > >> >> > > > However, when sending packets the rule of iot_addr is not applied >> >> (i.e. >> >> > > all >> >> > > > UDP packets on port 9999 are accepted regardless of the iot_addr >> >> value). >> >> > > > >> >> > > > When I dump the flows on s1 it does not return the iot_addr part of >> >> the >> >> > > > rule (checked by debugging). So it seems that the switch didn't >> >> accept my >> >> > > > new field and created the table entry without it, therefore >> >> iot_addr is >> >> > > not >> >> > > > being matched against when a packet arrives. >> >> > > > >> >> > > > I made sure that the switch is running the OpenFlow15 version using >> >> the >> >> > > -O >> >> > > > command. >> >> > > > >> >> > > > Is there something I am missing ? >> >> > > > >> >> > > > >> >> > > > >> >> > > > On Sun, Mar 13, 2016 at 7:16 PM, Ben Pfaff <b...@ovn.org> wrote: >> >> > > > >> >> > > > > On Sun, Mar 13, 2016 at 05:32:45PM +0300, Enas Ahmad wrote: >> >> > > > > > Thanks Ben for the useful answer, following these directions I >> >> was >> >> > > able >> >> > > > > to >> >> > > > > > add a new field and was able to successfully create a table >> >> entry >> >> > > > > > specifying a value for that filed. >> >> > > > > > >> >> > > > > > However, now I need to verify the packet extraction process >> >> done in >> >> > > > > > miniflow_extract(). My question is: how can I debug this method >> >> ? >> >> > > > > > flow.c does not import openvswitch/vlog.h, is there a reason for >> >> > > that ? >> >> > > > > > should I just add support for logging in flow.c or is there >> >> another >> >> > > way >> >> > > > > to >> >> > > > > > debug it ? >> >> > > > > >> >> > > > > Just add logging to flow.c >> >> > > > > >> >> > > > >> >> > > > -- >> >> > > > >> >> > > > ------------------------------ >> >> > > > This message and its contents, including attachments are intended >> >> solely >> >> > > > for the original recipient. If you are not the intended recipient >> >> or have >> >> > > > received this message in error, please notify me immediately and >> >> delete >> >> > > > this message from your computer system. Any unauthorized use or >> >> > > > distribution is prohibited. Please consider the environment before >> >> > > printing >> >> > > > this email. >> >> > > >> >> > >> >> > -- >> >> > >> >> > ------------------------------ >> >> > This message and its contents, including attachments are intended solely >> >> > for the original recipient. If you are not the intended recipient or >> >> have >> >> > received this message in error, please notify me immediately and delete >> >> > this message from your computer system. Any unauthorized use or >> >> > distribution is prohibited. Please consider the environment before >> >> printing >> >> > this email. >> >> >> > >> > >> >> -- >> >> ------------------------------ >> This message and its contents, including attachments are intended solely >> for the original recipient. If you are not the intended recipient or have >> received this message in error, please notify me immediately and delete >> this message from your computer system. Any unauthorized use or >> distribution is prohibited. Please consider the environment before printing >> this email. _______________________________________________ discuss mailing list discuss@openvswitch.org http://openvswitch.org/mailman/listinfo/discuss
