diff mbox

[ovs-dev,v2,02/15] netdev: Return number of packet from netdev_pop_header()

Message ID 1461290070-63896-3-git-send-email-pshelar@ovn.org
State Superseded
Headers show

Commit Message

Pravin Shelar April 22, 2016, 1:54 a.m. UTC 
Current tunnel-pop API does not allow the netdev implementation
retain a packet but STT can keep a packet from batch of packets
during TCP reassembly processing. To return exact count of
valid packet STT need to pass this number of packet parameter
as a reference.

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
 lib/dpif-netdev.c       |  9 +++++++--
 lib/netdev-native-tnl.c | 41 +++++++++++++++++++++++++----------------
 lib/netdev-native-tnl.h |  6 +++---
 lib/netdev-provider.h   |  6 ++++--
 lib/netdev.c            | 14 ++++++--------
 lib/netdev.h            |  2 +-
 6 files changed, 46 insertions(+), 32 deletions(-)

Comments

Jesse Gross May 6, 2016, 7:35 p.m. UTC | #1 
On Thu, Apr 21, 2016 at 6:54 PM, Pravin B Shelar <pshelar@ovn.org> wrote:
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 1e8a37c..5dcb862 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
>      case OVS_ACTION_ATTR_OUTPUT:
> @@ -3775,8 +3774,12 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
>                     packets = tnl_pkt;
>                  }
>
> -                err = netdev_pop_header(p->netdev, packets, cnt);
> +                err = netdev_pop_header(p->netdev, packets, &cnt);
> +                if (!cnt) {
> +                    return;
> +                }
>                  if (!err) {

I think that there is a difference in the handling of freeing packets
based on the type of error that we encounter.

If the protocol handler runs into an error on a single packet, it will
call dp_packet_delete(). That seems correct to me.

However, if tunnel header popping is completely unsupported then
netdev_pop_handler() will return an error code. This will result in
dp_netdev_drop_packets() being called, which will only free the
packets if may_steal is false. This was presumably done because that's
the only case where we clone but if may_steal is true then we're still
expect to take ownership of the packet.

This problem wasn't introduced in this patch but I think we could
simplify things a little and avoid it. Right now, netdev_pop_header()
effectively has two methods of indicating an error - the error code
and cnt set to 0. We could make it just free the packets in the event
that the pop action isn't supported and not make the caller worry
about the details.
Pravin Shelar May 9, 2016, 5:36 p.m. UTC | #2 
On Fri, May 6, 2016 at 12:35 PM, Jesse Gross <jesse@kernel.org> wrote:
> On Thu, Apr 21, 2016 at 6:54 PM, Pravin B Shelar <pshelar@ovn.org> wrote:
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 1e8a37c..5dcb862 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>>      case OVS_ACTION_ATTR_OUTPUT:
>> @@ -3775,8 +3774,12 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
>>                     packets = tnl_pkt;
>>                  }
>>
>> -                err = netdev_pop_header(p->netdev, packets, cnt);
>> +                err = netdev_pop_header(p->netdev, packets, &cnt);
>> +                if (!cnt) {
>> +                    return;
>> +                }
>>                  if (!err) {
>
> I think that there is a difference in the handling of freeing packets
> based on the type of error that we encounter.
>
> If the protocol handler runs into an error on a single packet, it will
> call dp_packet_delete(). That seems correct to me.
>
> However, if tunnel header popping is completely unsupported then
> netdev_pop_handler() will return an error code. This will result in
> dp_netdev_drop_packets() being called, which will only free the
> packets if may_steal is false. This was presumably done because that's
> the only case where we clone but if may_steal is true then we're still
> expect to take ownership of the packet.
>
> This problem wasn't introduced in this patch but I think we could
> simplify things a little and avoid it. Right now, netdev_pop_header()
> effectively has two methods of indicating an error - the error code
> and cnt set to 0. We could make it just free the packets in the event
> that the pop action isn't supported and not make the caller worry
> about the details.

ok, I would add a patch to fix this issue later in the series.
diff mbox

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1e8a37c..5dcb862 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3724,7 +3724,6 @@  dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
     struct dp_netdev *dp = pmd->dp;
     int type = nl_attr_type(a);
     struct dp_netdev_port *p;
-    int i;
 
     switch ((enum ovs_action_attr)type) {
     case OVS_ACTION_ATTR_OUTPUT:
@@ -3775,8 +3774,12 @@  dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
                    packets = tnl_pkt;
                 }
 
-                err = netdev_pop_header(p->netdev, packets, cnt);
+                err = netdev_pop_header(p->netdev, packets, &cnt);
+                if (!cnt) {
+                    return;
+                }
                 if (!err) {
+                    int i;
 
                     for (i = 0; i < cnt; i++) {
                         packets[i]->md.in_port.odp_port = portno;
@@ -3799,6 +3802,7 @@  dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
             struct ofpbuf actions;
             struct flow flow;
             ovs_u128 ufid;
+            int i;
 
             userdata = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_USERDATA);
             ofpbuf_init(&actions, 0);
@@ -3830,6 +3834,7 @@  dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
     case OVS_ACTION_ATTR_RECIRC:
         if (*depth < MAX_RECIRC_DEPTH) {
             struct dp_packet *recirc_pkts[NETDEV_MAX_BURST];
+            int i;
 
             if (!may_steal) {
                dp_netdev_clone_pkt_batch(recirc_pkts, packets, cnt);
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index b52b068..d28cfbf 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -353,7 +353,7 @@  parse_gre_header(struct dp_packet *packet,
     return hlen;
 }
 
-int
+struct dp_packet *
 netdev_gre_pop_header(struct dp_packet *packet)
 {
     struct pkt_metadata *md = &packet->md;
@@ -365,17 +365,20 @@  netdev_gre_pop_header(struct dp_packet *packet)
 
     pkt_metadata_init_tnl(md);
     if (hlen > dp_packet_size(packet)) {
-        return EINVAL;
+        goto err;
     }
 
     hlen = parse_gre_header(packet, tnl);
     if (hlen < 0) {
-        return -hlen;
+        goto err;
     }
 
     dp_packet_reset_packet(packet, hlen);
 
-    return 0;
+    return packet;
+err:
+    dp_packet_delete(packet);
+    return NULL;
 }
 
 void
@@ -450,7 +453,7 @@  netdev_gre_build_header(const struct netdev *netdev,
     return 0;
 }
 
-int
+struct dp_packet *
 netdev_vxlan_pop_header(struct dp_packet *packet)
 {
     struct pkt_metadata *md = &packet->md;
@@ -460,12 +463,12 @@  netdev_vxlan_pop_header(struct dp_packet *packet)
 
     pkt_metadata_init_tnl(md);
     if (VXLAN_HLEN > dp_packet_l4_size(packet)) {
-        return EINVAL;
+        goto err;
     }
 
     vxh = udp_extract_tnl_md(packet, tnl, &hlen);
     if (!vxh) {
-        return EINVAL;
+        goto err;
     }
 
     if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) ||
@@ -473,14 +476,17 @@  netdev_vxlan_pop_header(struct dp_packet *packet)
         VLOG_WARN_RL(&err_rl, "invalid vxlan flags=%#x vni=%#x\n",
                      ntohl(get_16aligned_be32(&vxh->vx_flags)),
                      ntohl(get_16aligned_be32(&vxh->vx_vni)));
-        return EINVAL;
+        goto err;
     }
     tnl->tun_id = htonll(ntohl(get_16aligned_be32(&vxh->vx_vni)) >> 8);
     tnl->flags |= FLOW_TNL_F_KEY;
 
     dp_packet_reset_packet(packet, hlen + VXLAN_HLEN);
 
-    return 0;
+    return packet;
+err:
+    dp_packet_delete(packet);
+    return NULL;
 }
 
 int
@@ -508,7 +514,7 @@  netdev_vxlan_build_header(const struct netdev *netdev,
     return 0;
 }
 
-int
+struct dp_packet *
 netdev_geneve_pop_header(struct dp_packet *packet)
 {
     struct pkt_metadata *md = &packet->md;
@@ -520,12 +526,12 @@  netdev_geneve_pop_header(struct dp_packet *packet)
     if (GENEVE_BASE_HLEN > dp_packet_l4_size(packet)) {
         VLOG_WARN_RL(&err_rl, "geneve packet too small: min header=%u packet size=%"PRIuSIZE"\n",
                      (unsigned int)GENEVE_BASE_HLEN, dp_packet_l4_size(packet));
-        return EINVAL;
+        goto err;
     }
 
     gnh = udp_extract_tnl_md(packet, tnl, &ulen);
     if (!gnh) {
-        return EINVAL;
+        goto err;
     }
 
     opts_len = gnh->opt_len * 4;
@@ -533,18 +539,18 @@  netdev_geneve_pop_header(struct dp_packet *packet)
     if (hlen > dp_packet_size(packet)) {
         VLOG_WARN_RL(&err_rl, "geneve packet too small: header len=%u packet size=%u\n",
                      hlen, dp_packet_size(packet));
-        return EINVAL;
+        goto err;
     }
 
     if (gnh->ver != 0) {
         VLOG_WARN_RL(&err_rl, "unknown geneve version: %"PRIu8"\n", gnh->ver);
-        return EINVAL;
+        goto err;
     }
 
     if (gnh->proto_type != htons(ETH_TYPE_TEB)) {
         VLOG_WARN_RL(&err_rl, "unknown geneve encapsulated protocol: %#x\n",
                      ntohs(gnh->proto_type));
-        return EINVAL;
+        goto err;
     }
 
     tnl->flags |= gnh->oam ? FLOW_TNL_F_OAM : 0;
@@ -557,7 +563,10 @@  netdev_geneve_pop_header(struct dp_packet *packet)
 
     dp_packet_reset_packet(packet, hlen);
 
-    return 0;
+    return packet;
+err:
+    dp_packet_delete(packet);
+    return NULL;
 }
 
 int
diff --git a/lib/netdev-native-tnl.h b/lib/netdev-native-tnl.h
index 5173b10..dbe6bd0 100644
--- a/lib/netdev-native-tnl.h
+++ b/lib/netdev-native-tnl.h
@@ -29,7 +29,7 @@  netdev_gre_build_header(const struct netdev *netdev,
 void
 netdev_gre_push_header(struct dp_packet *packet,
                        const struct ovs_action_push_tnl *data);
-int
+struct dp_packet *
 netdev_gre_pop_header(struct dp_packet *packet);
 
 void
@@ -39,14 +39,14 @@  int
 netdev_geneve_build_header(const struct netdev *netdev,
                            struct ovs_action_push_tnl *data,
                            const struct flow *tnl_flow);
-int
+struct dp_packet *
 netdev_geneve_pop_header(struct dp_packet *packet);
 
 int
 netdev_vxlan_build_header(const struct netdev *netdev,
                           struct ovs_action_push_tnl *data,
                           const struct flow *tnl_flow);
-int
+struct dp_packet *
 netdev_vxlan_pop_header(struct dp_packet *packet);
 
 void
diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index c2ddbf1..6af0708 100644
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -275,8 +275,10 @@  struct netdev_class {
                         const struct ovs_action_push_tnl *data);
 
     /* Pop tunnel header from packet, build tunnel metadata and resize packet
-     * for further processing. */
-    int (*pop_header)(struct dp_packet *packet);
+     * for further processing.
+     * Returns NULL in case of error or tunnel implementation queued packet for further
+     * processing. */
+    struct dp_packet * (*pop_header)(struct dp_packet *packet);
 
     /* Returns the id of the numa node the 'netdev' is on.  If there is no
      * such info, returns NETDEV_NUMA_UNSPEC. */
diff --git a/lib/netdev.c b/lib/netdev.c
index 3e50694..cac91ea 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -767,23 +767,21 @@  netdev_send(struct netdev *netdev, int qid, struct dp_packet **buffers,
 }
 
 int
-netdev_pop_header(struct netdev *netdev, struct dp_packet **buffers, int cnt)
+netdev_pop_header(struct netdev *netdev, struct dp_packet **buffers, int *pcnt)
 {
-    int i;
+    int i, cnt = *pcnt, n_cnt = 0;
 
     if (!netdev->netdev_class->pop_header) {
         return EOPNOTSUPP;
     }
 
     for (i = 0; i < cnt; i++) {
-        int err;
-
-        err = netdev->netdev_class->pop_header(buffers[i]);
-        if (err) {
-            dp_packet_clear(buffers[i]);
+        buffers[i] = netdev->netdev_class->pop_header(buffers[i]);
+        if (buffers[i]) {
+            buffers[n_cnt++] = buffers[i];
         }
     }
-
+    *pcnt = n_cnt;
     return 0;
 }
 
diff --git a/lib/netdev.h b/lib/netdev.h
index 6dabc92..f31b565 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -159,7 +159,7 @@  int netdev_push_header(const struct netdev *netdev,
                        struct dp_packet **buffers, int cnt,
                        const struct ovs_action_push_tnl *data);
 int netdev_pop_header(struct netdev *netdev, struct dp_packet **buffers,
-                      int cnt);
+                      int *pcnt);
 
 /* Hardware address. */
 int netdev_set_etheraddr(struct netdev *, const struct eth_addr mac);