Thanks Li,Rongqing
On Mon, Nov 18, 2019 at 9:35 PM Li,Rongqing <lirongq...@baidu.com> wrote:
> Thanks, Darrell
>
>
>
> I can try to test it
>
thanks !; can you try this 2 patch series ?
dball@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 <dlu...@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: d...@openvswitch.org
Cache the batch action list in the batch itself. This will be
used in a subsequent patch.
Signed-off-by: Darrell Ball <dlu...@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@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 <dlu...@gmail.com>
Date: Sun, 24 Nov 2019 17:33:58 -0800
Subject: [patch v1 2/2] ipf: Resume fragments in same action list.
To: d...@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 <dlu...@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:dlu...@gmail.com]
> *发送时间:* 2019年11月19日 10:22
> *收件人:* Li,Rongqing <lirongq...@baidu.com>
> *抄送:* ovs dev <d...@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 <dlu...@gmail.com> wrote:
>
>
>
>
>
> On Fri, Nov 15, 2019 at 6:03 PM Li,Rongqing <lirongq...@baidu.com> wrote:
>
> 发件人: Darrell Ball <dlu...@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
>
>
>
>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
