[ovs-dev] ipf question

Darrell Ball dlu998 at gmail.com
Tue Nov 26 02:37:32 UTC 2019


Thanks Li,Rongqing

On Mon, Nov 18, 2019 at 9:35 PM Li,Rongqing <lirongqing at baidu.com> wrote:

> Thanks, Darrell
>
>
>
> I can try to test it
>

thanks !; can you try this 2 patch series ?

dball at ubuntu:~/openvswitch/ovs$ cat
outgoing2/0001-dp-packet-Cache-batch-action-list-in-batch.patch
>From 80436fb57a54ecfd532e99086dbb0e5142415070 Mon Sep 17 00:00:00 2001
From: Darrell Ball <dlu998 at gmail.com>
Date: Sun, 24 Nov 2019 17:29:07 -0800
Subject: [patch v1 1/2] dp-packet: Cache batch action list in batch.
To: dev at openvswitch.org

Cache the batch action list in the batch itself. This will be
used in a subsequent patch.

Signed-off-by: Darrell Ball <dlu998 at gmail.com>
---
 lib/dp-packet.h | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 14f0897..77df801 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -775,9 +775,11 @@ enum { NETDEV_MAX_BURST = 32 }; /* Maximum number
packets in a batch. */

 struct dp_packet_batch {
     size_t count;
+    size_t actions_len;
+    const struct nlattr *actions;
+    struct dp_packet *packets[NETDEV_MAX_BURST];
     bool trunc; /* true if the batch needs truncate. */
     bool do_not_steal; /* Indicate that the packets should not be stolen.
*/
-    struct dp_packet *packets[NETDEV_MAX_BURST];
 };

 static inline void
@@ -786,6 +788,8 @@ dp_packet_batch_init(struct dp_packet_batch *batch)
     batch->count = 0;
     batch->trunc = false;
     batch->do_not_steal = false;
+    batch->actions = NULL;
+    batch->actions_len = 0;
 }

 static inline void
@@ -930,6 +934,27 @@ dp_packet_batch_reset_cutlen(struct dp_packet_batch
*batch)
     }
 }

+static inline void
+dp_packet_batch_set_action_ctx(struct dp_packet_batch *batch,
+                               const struct nlattr *actions,
+                               size_t actions_len)
+{
+    batch->actions = actions;
+    batch->actions_len = actions_len;
+}
+
+static inline const struct nlattr *
+dp_packet_batch_get_actions(struct dp_packet_batch *batch)
+{
+    return batch->actions;
+}
+
+static inline size_t
+dp_packet_batch_get_action_len(struct dp_packet_batch *batch)
+{
+    return batch->actions_len;
+}
+
 #ifdef  __cplusplus
 }
 #endif
-- 
1.9.1

dball at ubuntu:~/openvswitch/ovs$ cat
outgoing2/0002-ipf-Resume-fragments-in-same-action-list.patch
>From 4aefd4e791f9d7b6ef916e41136cb59e6020bb22 Mon Sep 17 00:00:00 2001
From: Darrell Ball <dlu998 at gmail.com>
Date: Sun, 24 Nov 2019 17:33:58 -0800
Subject: [patch v1 2/2] ipf: Resume fragments in same action list.
To: dev at openvswitch.org

Once fragments are reassembled and go through conntrack, the fragments
need to resume processing in the same action list.  There is a
semantic requirement that the list have, at most, one conntrack action,
so this specifies the resume point in the list as well.  A memcmp is
used to compare pre and post processing lists as padding is zeroed out,
therefore having predictable values.

Signed-off-by: Darrell Ball <dlu998 at gmail.com>
---
 lib/dpif-netdev.c |  1 +
 lib/ipf.c         | 49 ++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5142bad..3ddca5b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7363,6 +7363,7 @@ dp_netdev_execute_actions(struct dp_netdev_pmd_thread
*pmd,
 {
     struct dp_netdev_execute_aux aux = { pmd, flow };

+    dp_packet_batch_set_action_ctx(packets, actions, actions_len);
     odp_execute_actions(&aux, packets, should_steal, actions,
                         actions_len, dp_execute_cb);
 }
diff --git a/lib/ipf.c b/lib/ipf.c
index 45c4891..ad82620 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -91,6 +91,8 @@ union ipf_addr {
 /* Represents a single fragment; part of a list of fragments. */
 struct ipf_frag {
     struct dp_packet *pkt;
+    struct nlattr *actions;
+    size_t actions_len;
     uint16_t start_data_byte;
     uint16_t end_data_byte;
     bool dnsteal; /* 'do not steal': if true, ipf should not free packet.
*/
@@ -261,7 +263,12 @@ ipf_list_clean(struct hmap *frag_lists,
 {
     ovs_list_remove(&ipf_list->list_node);
     hmap_remove(frag_lists, &ipf_list->node);
-    free(ipf_list->frag_list);
+    struct ipf_frag *frag_list = ipf_list->frag_list;
+    ovs_assert(frag_list);
+    for (int i = 0; i <= ipf_list->last_inuse_idx; i++) {
+        free(frag_list[i].actions);
+    }
+    free(frag_list);
     free(ipf_list);
 }

@@ -793,7 +800,7 @@ static bool
 ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
                  struct dp_packet *pkt, uint16_t start_data_byte,
                  uint16_t end_data_byte, bool ff, bool lf, bool v6,
-                 bool dnsteal)
+                 struct dp_packet_batch *pb)
     OVS_REQUIRES(ipf->ipf_lock)
 {
     bool duped_frag = ipf_is_frag_duped(ipf_list->frag_list,
@@ -811,7 +818,11 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list
*ipf_list,
             frag->pkt = pkt;
             frag->start_data_byte = start_data_byte;
             frag->end_data_byte = end_data_byte;
-            frag->dnsteal = dnsteal;
+            frag->dnsteal = pb->do_not_steal;
+            frag->actions_len = dp_packet_batch_get_action_len(pb);
+            ovs_assert(dp_packet_batch_get_actions(pb));
+            frag->actions = xmemdup(dp_packet_batch_get_actions(pb),
+                                    frag->actions_len);
             ipf_list->last_inuse_idx++;
             atomic_count_inc(&ipf->nfrag);
             ipf_count(ipf, v6, IPF_NFRAGS_ACCEPTED);
@@ -849,7 +860,7 @@ ipf_list_init(struct ipf_list *ipf_list, struct
ipf_list_key *key,
 static bool
 ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type,
                 uint16_t zone, long long now, uint32_t hash_basis,
-                bool dnsteal)
+                struct dp_packet_batch *pb)
     OVS_REQUIRES(ipf->ipf_lock)
 {
     struct ipf_list_key key;
@@ -918,7 +929,7 @@ ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt,
ovs_be16 dl_type,
     }

     return ipf_process_frag(ipf, ipf_list, pkt, start_data_byte,
-                            end_data_byte, ff, lf, v6, dnsteal);
+                            end_data_byte, ff, lf, v6, pb);
 }

 /* Filters out fragments from a batch of fragments and adjust the batch. */
@@ -940,7 +951,7 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct
dp_packet_batch *pb,

             ovs_mutex_lock(&ipf->ipf_lock);
             if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis,
-                                 pb->do_not_steal)) {
+                                 pb)) {
                 dp_packet_batch_refill(pb, pkt, pb_idx);
             }
             ovs_mutex_unlock(&ipf->ipf_lock);
@@ -954,20 +965,30 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct
dp_packet_batch *pb,
  * management has trouble dealing with multiple source types.  The
  * check_source paramater is used to indicate when this check is needed. */
 static bool
-ipf_dp_packet_batch_add(struct dp_packet_batch *pb , struct dp_packet *pkt,
-                        bool check_source OVS_UNUSED)
+ipf_dp_packet_batch_add(struct dp_packet_batch *pb, struct dp_packet *pkt,
+                        struct ipf_frag *frag, bool check_source
OVS_UNUSED)
 {
 #ifdef DPDK_NETDEV
     if ((dp_packet_batch_is_full(pb)) ||
         /* DPDK cannot handle multiple sources in a batch. */
         (check_source && !dp_packet_batch_is_empty(pb)
-         && pb->packets[0]->source != pkt->source)) {
+         && pb->packets[0]->source != frag->pkt->source)) {
 #else
     if (dp_packet_batch_is_full(pb)) {
 #endif
         return false;
     }

+    /* Make sure that the fragment resumes processing in the same action
list.
+     * There is a semantic requirement that an action list have, at most,
one
+     * conntrack action, which could be enforced when the action list is
+     * generated.  Netlink padding is predictably zero, hence memcmp should
+     * be reliable to compare action lists. */
+    if ((dp_packet_batch_get_action_len(pb) != frag->actions_len) ||
+        (memcmp(dp_packet_batch_get_actions(pb), frag->actions,
+                frag->actions_len))) {
+        return false;
+    }
     dp_packet_batch_add(pb, pkt);
     return true;
 }
@@ -1020,9 +1041,11 @@ ipf_send_frags_in_list(struct ipf *ipf, struct
ipf_list *ipf_list,
     }

     while (ipf_list->last_sent_idx < ipf_list->last_inuse_idx) {
-        struct dp_packet *pkt
-            = ipf_list->frag_list[ipf_list->last_sent_idx + 1].pkt;
-        if (ipf_dp_packet_batch_add(pb, pkt, true)) {
+        struct ipf_frag *frag =
+            &ipf_list->frag_list[ipf_list->last_sent_idx + 1];
+        struct dp_packet *pkt = frag->pkt;
+
+        if (ipf_dp_packet_batch_add(pb, pkt, frag, true)) {
             ipf_list->last_sent_idx++;
             atomic_count_dec(&ipf->nfrag);

@@ -1122,7 +1145,7 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct
dp_packet_batch *pb)

     LIST_FOR_EACH_SAFE (rp, next, rp_list_node,
&ipf->reassembled_pkt_list) {
         if (!rp->list->reass_execute_ctx &&
-            ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
+            ipf_dp_packet_batch_add(pb, rp->pkt, rp->list->frag_list,
false)) {
             rp->list->reass_execute_ctx = rp->pkt;
         }
     }
-- 
1.9.1


>
>
> Other question, it seems that ipf is driven by input packet, no timer,  if
> a packet has 40 fragment, first time, 32 fragments are received, next time,
> 8 fragment are received,  reassemble is success, and send out 32 fragment.
> After that,  if no new packets are received, the remaining 8 fragment will
> in complete list, and can not send out until expired to clean them, is it
> true?
>

Fragments will continually try to be sent out to resume to a same type
action list execution batch and point of processing as they were received
from;
At least, there would be subsequent batches for retries.


>
>
> -RongQing
>
>
>
> *发件人:* Darrell Ball [mailto:dlu998 at gmail.com]
> *发送时间:* 2019年11月19日 10:22
> *收件人:* Li,Rongqing <lirongqing at baidu.com>
> *抄送:* ovs dev <dev at openvswitch.org>
> *主题:* Re: ipf question
>
>
>
> Thanks; I had a look and I noticed ipf does not keep all the context it
> needs
>
> to properly resume fragment processing in the general case; I have a
> potential fix,
>
> but won't get to it this week.
>
>
>
>
>
> On Sun, Nov 17, 2019 at 11:08 PM Darrell Ball <dlu998 at gmail.com> wrote:
>
>
>
>
>
> On Fri, Nov 15, 2019 at 6:03 PM Li,Rongqing <lirongqing at baidu.com> wrote:
>
> 发件人: Darrell Ball <dlu998 at gmail.com>
> 发送时间: 2019年11月15日 22:58
> 收件人: Li,Rongqing
> 抄送: ovs dev
> 主题: Re: ipf question
>
>
> >Let me paraphrase, just to confirm we are on the same page.
> >IIUC, for example, in the case of a 33 fragment packet, in the first pass
> all 33 fragments enter ipf, then are >reassembled, pass thru conntrack
> >and then the frags sent out, while in the second pass, only 32 fragments
> enter conntrack/ipf, while index 32 >fragment is being forwarded out
> without going thru conntrack/ipf ?
>
> true.
> the second pass is recirculation
> and index 32 fragment is not into contrack/ipf, and send out to vm
> directly.
>
>
>
> can you check what rule is being hit by that fragment packet vs others and
> then compare the pkt metadata
>
>
>
>
> if  I change  NETDEV_MAX_BURST to 64, it works
>
>
>
> good test
>
>
>
>
> thanks
>
> -RongQing
>
>
>
>
>
>


More information about the dev mailing list