On 9 March 2017 at 10:25, Yi-Hung Wei <[email protected]> wrote:
> Currently, a controller may potentially trigger a segmentation fault if it
> accidentally removes a TLV mapping that is still used by an active flow.
> To resolve this issue, in this patch, we maintain reference counting for each
> dynamically allocated variable length mf_fields, so that vswitchd can use this
> information to properly remove a TLV mapping, and to return an error if the
> controller tries to remove a TLV mapping that is still used by any active
> flow.
>
> To keep track of the usage of tun_metadata for each flow, two 'uint64_t'
> bitmaps are introduce for the flow match and flow action respectively. We use
> 'uint64_t' as a bitmap since the 64 geneve TLV tunnel metadata are the only
> available variable length mf_fields for now. We shall adopt general bitmap
> when
> more variable length mf_fields are introduced. The bitmaps are configured
> during the flow decoding process, and vswitchd use these bitmaps to increase
> or
> decrease the ref counting when the flow is created or deleted.
>
> VMWare-BZ: #1768370
> Fixes: 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs.")
> Suggested-by: Jarno Rajahalme <[email protected]>
> Suggested-by: Joe Stringer <[email protected]>
> Signed-off-by: Yi-Hung Wei <[email protected]>
> ---
Couple of minor comments to add from previous discussion,
<snip>
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 40704e628aaa..e844008f6294 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -27,6 +27,8 @@
> #include "openvswitch/dynamic-string.h"
> #include "nx-match.h"
> #include "openvswitch/ofp-util.h"
> +#include "ofproto/ofproto-provider.h"
We can drop this include now.
<snip>
> @@ -2697,53 +2710,98 @@ mf_get_vl_mff(const struct mf_field *mff,
> return NULL;
> }
>
> -/* Updates the tun_metadata mf_field in 'vl_mff_map' according to 'ttm'.
> - * This function is supposed to be invoked after tun_metadata_table_mod(). */
> -enum ofperr
> -mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
> - const struct ofputil_tlv_table_mod *ttm)
> +static enum ofperr
> +mf_vl_mff_map_del(struct vl_mff_map *vl_mff_map,
> + const struct ofputil_tlv_table_mod *ttm, bool force)
> OVS_REQUIRES(vl_mff_map->mutex)
> {
> struct ofputil_tlv_map *tlv_map;
> + struct vl_mf_field *vmf;
> + unsigned int idx;
>
> - if (ttm->command == NXTTMC_CLEAR) {
> - mf_vl_mff_map_clear(vl_mff_map);
> - return 0;
> + if (!force) {
> + LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> + idx = MFF_TUN_METADATA0 + tlv_map->index;
Idly wondering whether it improves code readability to have some
simple functions like
mff_to_idx(int)
idx_to_mff(int)
which handle the +/- of MFF_TUN_METADATA0 in so many places around the code.
If you think it's worthwhile it could be a separate followup patch.
> @@ -6332,6 +6365,11 @@ ofpacts_pull_openflow_actions__(struct ofpbuf
> *openflow,
> * you should call ofpacts_pull_openflow_instructions() instead of this
> * function.
> *
> + * 'vl_mff_map' and 'ofpacts_tlv_bitmap' are optional. If 'vl_mff_map' is not
> + * NULL, it is used to drive the variable length mf_fields, and
Where does it 'drive' the variable to? :-)
(This comment might benefit from the feedback I had on similar
comments in the previous patch)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
