> On Mar 17, 2015, at 2:01 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> On Fri, Mar 13, 2015 at 04:52:00PM -0700, Jarno Rajahalme wrote:
>> xlate_actions() now considers an optional recirculation context (via
>> 'xin') and restores OpenFlow pipeline metadata (registers, 'metadata',
>> etc.) based on it. The recirculation context may contain an action
>> set and stack to be restored and further actions to be executed upon
>> recirculation. It also contains a table_id number to be used for rule
>> lookup in cases where no post-recirculation actions are used.
>>
>> The translation context internal metadata is restored using a new
>> internal action: UNROLL_XLATE action stores the translation context
>> data visible to OpenFlow controllers via PACKET_IN messages. This
>> includes the current table number and the current rule cookie.
>> UNROLL_XLATE actions are inserted only when the remaining actions may
>> generate PACKET_IN messages.
>>
>> These changes allow the post-MPLS recirculation to properly continue
>> with the pipeline metadata that existed at the time of recirculation.
>>
>> The internal table is still consulted for bonds.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> I'm a little concerned about ukey_create_from_dpif_flow(), which seems
> to get called whenever the datapath does not support ufids. It seems
> to reject recirculation actions in that case. Does that mean that
> userspace becomes incompatible (if any recirculation is attempted)
> with older datapaths? I think that is what happens, but I don't think
> that is the intent.
>
The only user of key_create_from_dpif_flow() has this comment just above:
/* Usually we try to avoid installing flows from revalidator threads,
* because locking on a umap may cause handler threads to block.
* However there are certain cases, like when ovs-vswitchd is
* restarted, where it is desirable to handle flows that exist in the
* datapath gracefully (ie, don't just clear the datapath). */
It seems to me that the dpif-layer always provides the ufid, by hashing from
the flow key. By looking at lib/dpif-netlink.c parse_odp_packet(), it seems
that the upcall’s ufid is always hashed from the datapath key, regardless of
the presence of an FLOW_ATTR_UKEY. So there should not be a problem here. Maybe
Joe could confirm this?
> Here in upcall_cb(), I think I understand the idea--that when we can't
> look up the recirculation ID, it's a problematic situation, so we
> shouldn't install its flow--but I'm not sure how that helps with
> anything. Doesn't that just mean that the problematic situation
> recurs slowly in userspace (sending lots of upcalls) instead of
> quickly in the datapath?
>
> + /* Prevent miss flow installation if the key has recirculation ID but we
> + * were not able to get a reference on it. */
> + if (type == DPIF_UC_MISS && upcall.recirc && !upcall.have_recirc_ref) {
> error = ENOSPC;
> + goto out;
> }
>
The inability to get a reference to a recirculation context only happens in a
rare case where all the datapath flows with references to the recirculation ID
are deleted while there is a concurrent upcall from one of these flows in
progress. In this case the reference count on the recirculation context has
reached zero before we get a reference on it. It is still safe to use the
recirculation context, as it is RCU protected. Any new packets received from
the original input port (i.e., not recirculated packets) will trigger an
allocation of a new recirculation ID, and after that the next recirculated
upcall will get a reference and install a flow. In short, there should not be a
case where we continuously process packets in the slow path due to this.
> In recirc_free_id, s/non-exiting/nonexistent/ in the log message.
Fixed.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
