Re: [ovs-dev] [bug4769] datapath: Get packet metadata from userspace in odp_packet_cmd_execute().

[Ben Pfaff]

On Tue, May 31, 2011 at 01:51:11PM -0700, Jesse Gross wrote:
> On Fri, May 27, 2011 at 4:40 PM, Ben Pfaff <b...@nicira.com> wrote:
> > Until now, the tun_id and in_port have been lost when a packet is sent from
> > the kernel to userspace and then back to the kernel. ??I didn't think that
> > this was a problem, but recent behavior made me look closer and see that
> > it makes a difference if sFlow is turned on or if an
> > ODP_ATTR_ACTION_CONTROLLER action is present. ??We could possibly kluge
> > around those, but for future-proofing it seems better to pass the packet
> > metadata from userspace to the kernel. ??That is what this commit does.
> >
> > This commit introduces a user-kernel protocol break. ??We could avoid that,
> > if it is desirable, by making ODP_PACKET_ATTR_KEY optional for
> > ODP_PACKET_CMD_EXECUTE commands.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > Bug #4769.
> 
> I think this bug number is from the original work that triggered
> looking into this but it is not actually related.

OK, I removed the bug number from the commit message.

> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 53574aa..88960ec 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -944,6 +944,7 @@ dpif_netdev_flow_dump_done(const struct dpif *dpif 
> > OVS_UNUSED, void *state_)
> >
> > ??static int
> > ??dpif_netdev_execute(struct dpif *dpif,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ??const struct nlattr *k OVS_UNUSED, size_t 
> > k_len OVS_UNUSED,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? const struct nlattr *actions, size_t 
> > actions_len,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? const struct ofpbuf *packet)
> > ??{
> 
> I think we need to extract the in port from the metadata here.
> There's no sFlow in userspace but it is still possible for a packet to
> be sent to the controller (there's the optimization that tries to
> avoid this case but it doesn't catch all situations).  If the packet
> does go to the controller, then it will have a port of -1 instead of
> the real one.  In addition, this would resolve an inconsistency
> between here and ofproto-dpif.c:send_packet() as to whether we should
> use -1 or 0 as the in port when we don't have a real one.

Good point.  It's easy to fix, anyhow.

Here's an incremental:

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 88960ec..c6f8603 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -944,7 +944,7 @@ dpif_netdev_flow_dump_done(const struct dpif *dpif 
OVS_UNUSED, void *state_)
 
 static int
 dpif_netdev_execute(struct dpif *dpif,
-                    const struct nlattr *k OVS_UNUSED, size_t k_len OVS_UNUSED,
+                    const struct nlattr *key_attrs, size_t key_len,
                     const struct nlattr *actions, size_t actions_len,
                     const struct ofpbuf *packet)
 {
@@ -963,6 +963,8 @@ dpif_netdev_execute(struct dpif *dpif,
         return error;
     }
 
+    dpif_netdev_flow_from_nlattrs(key_attrs, key_len, &key);
+
     if (mutates) {
         /* We need a deep copy of 'packet' since we're going to modify its
          * data. */
@@ -976,7 +978,7 @@ dpif_netdev_execute(struct dpif *dpif,
          * if we don't. */
         copy = *packet;
     }
-    flow_extract(&copy, 0, -1, &key);
+
     error = dp_netdev_execute_actions(dp, &copy, &key, actions, actions_len);
     if (mutates) {
         ofpbuf_uninit(&copy);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev