Joe,
I just noticed I never sent this out. Sorry, and thanks for the review!
Jarno
> On Feb 3, 2016, at 2:19 PM, Joe Stringer <j...@ovn.org> wrote:
>
> On 3 February 2016 at 12:33, Jarno Rajahalme <ja...@ovn.org> wrote:
>> From: Ethan Jackson <et...@nicira.com>
>>
>> There are certain use cases (such as bond rebalancing) where a
>> datapath flow's actions may change, while it's wildcard pattern
>> remains the same. Before this patch, revalidators would note the
>> change, delete the flow, and wait for the handlers to install an
>> updated version. This is inefficient, as many packets could get
>> punted to userspace before the new flow is finally installed.
>>
>> To improve the situation, this patch implements in place modification
>> of datapath flows. If the revalidators detect the only change to a
>> given ukey is its actions, instead of deleting it, it does a put with
>> the MODIFY flag set.
>>
>> This patch is a backport of commit 43b2f13 to branch-2.3.
>>
>> Signed-off-by: Ethan J. Jackson <et...@nicira.com>
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
>
> This also squashes in commit c56eba3b7ab0 ("ofproto-dpif-upcall: Don't
> delete modified ukeys.”)
Thanks, I added this note to the commit message.
> .
>
> <snip>
>
>> @@ -1543,62 +1611,68 @@ revalidate(struct revalidator *revalidator)
>> }
>>
>> dpif_flow_dump_state_uninit(udpif->dpif, state);
>> -}
>> -
>> -/* Called with exclusive access to 'revalidator' and 'ukey'. */
>> -static bool
>> -handle_missed_revalidation(struct revalidator *revalidator,
>> - struct udpif_key *ukey)
>> - OVS_NO_THREAD_SAFETY_ANALYSIS
>> -{
>> - struct udpif *udpif = revalidator->udpif;
>> - struct nlattr *mask, *actions;
>> - size_t mask_len, actions_len;
>> - struct dpif_flow_stats stats;
>> - struct ofpbuf *buf;
>> - bool keep = false;
>> -
>> - COVERAGE_INC(revalidate_missed_dp_flow);
>> -
>> - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf,
>> - &mask, &mask_len, &actions, &actions_len, &stats)) {
>> - keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
>> - actions_len, &stats);
>> - ofpbuf_delete(buf);
>> - }
>> -
>> - return keep;
>> + ofpbuf_uninit(&odp_actions);
>> }
>>
>> static void
>> revalidator_sweep__(struct revalidator *revalidator, bool purge)
>> OVS_NO_THREAD_SAFETY_ANALYSIS
>> {
>> + uint64_t odp_actions_stub[1024 / 8];
>> + struct ofpbuf odp_actions;
>> struct dump_op ops[REVALIDATE_MAX_BATCH];
>> struct udpif_key *ukey, *next;
>> size_t n_ops;
>>
>> n_ops = 0;
>>
>> + ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof
>> odp_actions_stub);
>> +
>> /* During garbage collection, this revalidator completely owns its ukeys
>> * map, and therefore doesn't need to do any locking. */
>> HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) {
>> if (ukey->flow_exists) {
>> bool missed_flow = !ukey->mark;
>> + enum reval_result result;
>> + struct nlattr *mask;
>> + size_t mask_len;
>> + struct ofpbuf *buf = NULL;
>>
>> ukey->mark = false;
>> - if (purge
>> - || (missed_flow
>> - && revalidator->udpif->need_revalidate
>> - && !handle_missed_revalidation(revalidator, ukey))) {
>> - struct dump_op *op = &ops[n_ops++];
>> -
>> - dump_op_init(op, ukey->key, ukey->key_len, ukey);
>> - if (n_ops == REVALIDATE_MAX_BATCH) {
>> - push_dump_ops(revalidator, ops, n_ops);
>> - n_ops = 0;
>> + result = purge ? UKEY_DELETE : UKEY_KEEP;
>> +
>> + if (missed_flow && revalidator->udpif->need_revalidate) {
>> + struct udpif *udpif = revalidator->udpif;
>> + struct nlattr *actions;
>> + size_t actions_len;
>> + struct dpif_flow_stats stats;
>> +
>> + COVERAGE_INC(revalidate_missed_dp_flow);
>> +
>> + if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len,
>> &buf,
>> + &mask, &mask_len, &actions, &actions_len,
>> + &stats)) {
>> + result = revalidate_ukey(udpif, ukey, mask, mask_len,
>> + actions, actions_len, &stats,
>> + &odp_actions);
>
>
> handle_missed_revalidation() would previously return the equivalent of
> UKEY_DELETE if the dpif_flow_get() fails. This would lead on to
> attempting flow deletion, which I'd expect is most likely also is
> going to fail, but it would also result in the ukey getting deleted
> (which we want). I suspect that with the current version above, if one
> runs 'ovs-dpctl del-flow...' then the corresponding ukeys will never
> be deleted.
>
> Simplest (same as current) behaviour is probably this incremental:
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 03b4afa7d2c5..ba0cd2f9b2d3 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1649,9 +1649,11 @@ revalidator_sweep__(struct revalidator
> *revalidator, bool purge)
>
> COVERAGE_INC(revalidate_missed_dp_flow);
>
> - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len,
> &buf,
> - &mask, &mask_len, &actions, &actions_len,
> - &stats)) {
> + if (dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len,
> &buf,
> + &mask, &mask_len, &actions, &actions_len,
> + &stats)) {
> + result = UKEY_DELETE;
> + } else {
> result = revalidate_ukey(udpif, ukey, mask, mask_len,
> actions, actions_len, &stats,
> &odp_actions);
>
>
Thanks, folded in for v2.
> Separately from this patch, I noticed a couple of other things:
>
> - I couldn't compile branch-2.3 without at least this patch for
> lib/type-props.h:
> 0f395fd366413208aac7072ef81b5aeda6a9e307
> (which depends on c002791a30818c2458599f993d1711e03566e7cc)
I cherry-picked these two for the v2.
> - I wonder if commit e83c93573b10 ("ofproto-dpif-upcall: Do not
> attribute stats when flow_del returns error.") is also a candidate for
> backport, but that's more of a statistics issue rather than
> reordering/correctness.
Done.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
