On 5/19/17, 1:15 PM, "Joe Stringer" <[email protected]> wrote:
On 17 May 2017 at 17:54, Joe Stringer <[email protected]> wrote:
> On 17 May 2017 at 16:26, Darrell Ball <[email protected]> wrote:
>>
>>
>> On 5/17/17, 2:19 PM, "Joe Stringer" <[email protected]> wrote:
>>
>> On 16 May 2017 at 21:01, Darrell Ball <[email protected]> wrote:
>> >
>> >
>> > On 5/15/17, 6:00 AM, "[email protected] on behalf of
Bodireddy, Bhanuprakash" <[email protected] on behalf of
[email protected]> wrote:
>> >
>> > >Commit daf4d3c18da4("odp: Support conntrack orig tuple key.")
introduced
>> > >new fields in struct 'pkt_metadata'. pkt_metadata_init() is
called for every
>> > >packet in the userspace datapath. When testing a simple
single flow case with
>> > >DPDK, we observe a lower throughput after the above commit
(it was 14.88
>> > >Mpps before, it is 13 Mpps after).
>> > >
>> > >This patch skips initializing ct_orig_tuple in
pkt_metadata_init().
>> > >It should be enough to initialize ct_state, because nobody
should look at
>> > >ct_orig_tuple unless ct_state is != 0.
>> > >
>> > >CC: Jarno Rajahalme <[email protected]>
>> > >Signed-off-by: Daniele Di Proietto <[email protected]>
>> > >---
>> > >I'm sending this as an RFC because I didn't check very
carefully if we can really
>> > >avoid initializing ct_orig_tuple.
>> > >
>> > >Maybe there are better solutions to this problem.
>> > >---
>> > > lib/packets.h | 2 +-
>> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > >diff --git a/lib/packets.h b/lib/packets.h index
a5a483bc8..6f1791c7a 100644
>> > >--- a/lib/packets.h
>> > >+++ b/lib/packets.h
>> > >@@ -129,7 +129,7 @@ pkt_metadata_init(struct pkt_metadata *md,
>> > >odp_port_t port)
>> > > /* It can be expensive to zero out all of the tunnel
metadata. However,
>> > > * we can just zero out ip_dst and the rest of the data
will never be
>> > > * looked at. */
>> > >- memset(md, 0, offsetof(struct pkt_metadata, in_port));
>> > >+ memset(md, 0, offsetof(struct pkt_metadata,
ct_orig_tuple));
>> > > md->tunnel.ip_dst = 0;
>> > > md->tunnel.ipv6_dst = in6addr_any;
>> > >
>> >
>> > It's been a while this RFC patch has been submitted to fix
performance drop on Master. This indeed fixes the OvS-DPDK performance drop
that was introduced by the conntrack commit.
>> > Is there a better fix than what is suggested above?
>> >
>> >
>> > This affects both kernel and userspace.
>> > I tested this on both datapaths.
>> > LGTM
>>
>> How do we make sure that ct_orig_tuple isn't used uninitialized? Do
we
>> need to zero out the ct_orig_tuple proto?
>>
>> There are a couple places where we explicitly set or reset the pkt
metadata ct_orig_tuple;
>> one is in pkt_metadata_from_flow().
>>
>> I know there is a check for ct_orig_tuple proto in
odp_key_from_pkt_metadata()
>> Can you find a case where can run this code without a set or reset of
ct_orig_tuple pkt md
>> or you are not sure ?
>
> I wasn't sure, and I didn't see a response to the question that
> Daniele asked below the dashes in the original submission.
>
> Is miniflow_extract() safe wrt this? Seems like plausibly if the
> recirc_id is nonzero but there is no CT state (because, eg, bonds, or
> MPLS pop to IP, etc) it could end up populating the miniflow with
> garbage. In particular the path from emc_processing() seems suspect.
Perhaps this is actually OK, because the ct_state would be initialized
to zero in pkt_metadata_init(), and the classifier would never look at
ct_orig_tuple unless the ct_state is valid (according to the
ovs-fields(7) documentation). In which case, it shouldn't(?) matter
that garbage is written to the miniflow.
I am glad you agree.
The way I was reasoning about this is:
ct_orig_tuple should not be looked at unless the connection tracker
has seen the packet. It would be a bug, otherwise.
Alternatively if md_is_valid
(eg, because it was recirculated),
s/eg/i.e./ for this code path
then it's up to the other callers
to ensure that md.ct_state was properly initialized. Effectively this
is done by the write_ct_md() function.
originates from conntrack in general.
I wonder though if actually the miniflow_extract() "ct_nw_proto_p"
initialization should only occur if the md->ct_state is valid.
That could save a few extra cycles in a similar vein to how this patch
proposes.
Thanks for verbalizing my offline suggestion.
The reason why this might be good to do is not just to save a few cycles, but
more importantly to enforce explicit logic about when ct_orig_tuple is relevant
in
a flow.
I sent a patch here
https://patchwork.ozlabs.org/patch/764989/
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
