On Mon, Aug 5, 2019 at 8:51 PM Darrell Ball <dlu...@gmail.com> wrote:
> Thanks for the patch
>
> The main comment I had from the V1 patch was adding the check
>
> + if (ofc->flags & NX_CT_F_COMMIT) {
>
> in compose_conntrack_action()
>
> I see that was done.
>
> After a quick scan, I had one minor comment inline.
>
I wanted to reiterate one more comment that I added to V1 regarding the
unwildcarding (inline).
>
> On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei <yihung....@gmail.com> wrote:
>
>> This patch derives the timeout policy based on ct zone from the
>> internal data structure that reads the configuration from ovsdb.
>>
>> Signed-off-by: Yi-Hung Wei <yihung....@gmail.com>
>> ---
>> lib/ct-dpif.c | 10 ++++++++++
>> lib/ct-dpif.h | 3 +++
>> lib/dpif-netdev.c | 1 +
>> lib/dpif-netlink.c | 10 ++++++++++
>> lib/dpif-provider.h | 5 +++++
>> ofproto/ofproto-dpif-xlate.c | 29 +++++++++++++++++++++++++++++
>> ofproto/ofproto-dpif.c | 24 ++++++++++++++++++++++++
>> ofproto/ofproto-provider.h | 5 +++++
>> ofproto/ofproto.c | 11 +++++++++++
>> ofproto/ofproto.h | 2 ++
>> 10 files changed, 100 insertions(+)
>>
>> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
>> index 7f9ce0a561f7..5d2acfd7810b 100644
>> --- a/lib/ct-dpif.c
>> +++ b/lib/ct-dpif.c
>> @@ -864,3 +864,13 @@ ct_dpif_timeout_policy_dump_done(struct dpif *dpif,
>> void *state)
>> ? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state)
>> : EOPNOTSUPP);
>> }
>> +
>> +int
>> +ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
>> + uint16_t dl_type, uint8_t nw_proto,
>> + struct ds *ds)
>> +{
>> + return (dpif->dpif_class->ct_format_timeout_policy_name
>> + ? dpif->dpif_class->ct_format_timeout_policy_name(
>> + dpif, tp_id, dl_type, nw_proto, ds) : EOPNOTSUPP);
>> +}
>> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
>> index 8dacb1c7c253..0a27568880c0 100644
>> --- a/lib/ct-dpif.h
>> +++ b/lib/ct-dpif.h
>> @@ -318,5 +318,8 @@ int ct_dpif_timeout_policy_dump_start(struct dpif
>> *dpif, void **statep);
>> int ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
>> struct ct_dpif_timeout_policy *tp);
>> int ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state);
>> +int ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
>> + uint16_t dl_type, uint8_t
>> nw_proto,
>> + struct ds *ds);
>>
>> #endif /* CT_DPIF_H */
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 7240a3e6f3c8..19cf9f21ec85 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -7539,6 +7539,7 @@ const struct dpif_class dpif_netdev_class = {
>> NULL, /* ct_timeout_policy_dump_start */
>> NULL, /* ct_timeout_policy_dump_next */
>> NULL, /* ct_timeout_policy_dump_done */
>> + NULL, /* ct_format_timeout_policy_name */
>> dpif_netdev_ipf_set_enabled,
>> dpif_netdev_ipf_set_min_frag,
>> dpif_netdev_ipf_set_max_nfrags,
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index b859508f718a..92da87027c58 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -3071,6 +3071,15 @@ dpif_netlink_format_tp_name(uint32_t id, uint16_t
>> l3num, uint8_t l4num,
>> ovs_assert(tp_name->length < CTNL_TIMEOUT_NAME_MAX);
>> }
>>
>> +static int
>> +dpif_netlink_ct_format_timeout_policy_name(struct dpif *dpif OVS_UNUSED,
>> + uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, struct ds *ds)
>> +{
>> + dpif_netlink_format_tp_name(tp_id,
>> + dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6, nw_proto, ds);
>> + return 0;
>> +}
>> +
>> #define CT_DPIF_NL_TP_TCP_MAPPINGS \
>> CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \
>> CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \
>> @@ -3891,6 +3900,7 @@ const struct dpif_class dpif_netlink_class = {
>> dpif_netlink_ct_timeout_policy_dump_start,
>> dpif_netlink_ct_timeout_policy_dump_next,
>> dpif_netlink_ct_timeout_policy_dump_done,
>> + dpif_netlink_ct_format_timeout_policy_name,
>> NULL, /* ipf_set_enabled */
>> NULL, /* ipf_set_min_frag */
>> NULL, /* ipf_set_max_nfrags */
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index 79a2314500cf..57b32ccb610f 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -536,6 +536,11 @@ struct dpif_class {
>> struct ct_dpif_timeout_policy
>> *tp);
>> int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
>>
>> + /* Get timeout policy name (OVS_CT_ATTR_TIMEOUT) from datapath. */
>> + int (*ct_format_timeout_policy_name)(struct dpif *, uint32_t tp_id,
>> + uint16_t dl_type, uint8_t
>> nw_proto,
>> + struct ds *ds);
>> +
>> /* IP Fragmentation. */
>>
>> /* Disables or enables conntrack fragment reassembly. The default
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 28a7fdd842a6..f9b517aaa270 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -28,6 +28,7 @@
>> #include "bond.h"
>> #include "bundle.h"
>> #include "byte-order.h"
>> +#include "ct-dpif.h"
>>
>
> move further down
>
>
>> #include "cfm.h"
>> #include "connmgr.h"
>> #include "coverage.h"
>> @@ -5977,6 +5978,30 @@ put_ct_helper(struct xlate_ctx *ctx,
>> }
>>
>> static void
>> +put_ct_timeout(struct ofpbuf *odp_actions, const char *dp_type,
>> + const struct ofproto_dpif *ofproto, const struct flow
>> *flow,
>> + struct flow_wildcards *wc, uint16_t zone_id)
>> +{
>> + struct dpif_backer *backer = ofproto->backer;
>> + uint32_t tp_id;
>> +
>> + if (ofproto_ct_zone_timeout_policy_get(&ofproto->up, zone_id,
>> &tp_id)) {
>> + if (!strcmp(dp_type, "system")) {
>> + struct ds ds = DS_EMPTY_INITIALIZER;
>> + int err = ct_dpif_format_timeout_policy_name(
>> + backer->dpif, tp_id, ntohs(flow->dl_type),
>> + flow->nw_proto, &ds);
>> + if (!err) {
>> + memset(&wc->masks.nw_proto, 0xff, sizeof
>> wc->masks.nw_proto);
>>
>
I think the above unwildcarding should be wrapped in a datapath type
specific function
in dpif-netlink or netlink-conntrack.
This is bcoz, the userspace datapath does not need to do this unwildcarding
since a timeout profile
can be shared across dl_type and ip_proto.
Also a note should be added to the function describing the why the
additional datapath flows are
needed (i.e. bcoz of Netfilter timeouts implementation). Note that these
datapath flows will remain if kept
minimally active, which is generally undesirable, hence needs to be flagged.
If this is going to delay things, it can be done later from my POV.
> + nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT,
>> + ds_cstr(&ds));
>> + }
>> + ds_destroy(&ds);
>> + }
>> + }
>> +}
>> +
>> +static void
>> put_ct_nat(struct xlate_ctx *ctx)
>> {
>> struct ofpact_nat *ofn = ctx->ct_nat_action;
>> @@ -6071,6 +6096,10 @@ compose_conntrack_action(struct xlate_ctx *ctx,
>> struct ofpact_conntrack *ofc,
>> put_ct_mark(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
>> put_ct_label(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
>> put_ct_helper(ctx, ctx->odp_actions, ofc);
>> + if (ofc->flags & NX_CT_F_COMMIT) {
>> + put_ct_timeout(ctx->odp_actions,
>> ctx->xbridge->ofproto->backer->type,
>> + ctx->xbridge->ofproto, &ctx->xin->flow, ctx->wc,
>> zone);
>> + }
>> put_ct_nat(ctx);
>> ctx->ct_nat_action = NULL;
>> nl_msg_end_nested(ctx->odp_actions, ct_offset);
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 6336494e0bc8..f31162f4481d 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -5351,6 +5351,29 @@ ct_zone_timeout_policy_reconfig(const struct
>> ofproto *ofproto,
>> }
>>
>> static bool
>> +ct_zone_timeout_policy_get(const struct ofproto *ofproto_,
>> + uint16_t zone, uint32_t *tp_id)
>> +{
>> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>> + struct dpif_backer *backer = ofproto->backer;
>> + struct ct_zone *ct_zone;
>> + struct ct_timeout_policy *ct_tp;
>> +
>> + ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
>> + if (!ct_zone) {
>> + return false;
>> + }
>> +
>> + ct_tp = ct_timeout_policy_lookup(&backer->ct_tps, &ct_zone->tp_uuid);
>> + if (!ct_tp) {
>> + return false;
>> + }
>> +
>> + *tp_id = ct_tp->cdtp.id;
>> + return true;
>> +}
>> +
>> +static bool
>> set_frag_handling(struct ofproto *ofproto_,
>> enum ofputil_frag_handling frag_handling)
>> {
>> @@ -6455,4 +6478,5 @@ const struct ofproto_class ofproto_dpif_class = {
>> ct_flush, /* ct_flush */
>> ct_zone_timeout_policy_reconfig,
>> ct_zone_timeout_policy_sweep,
>> + ct_zone_timeout_policy_get,
>> };
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index 41e07f0ee23e..1a2fc4a6a084 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -1880,6 +1880,11 @@ struct ofproto_class {
>> const struct ovsrec_datapath *dp_cfg, unsigned int
>> idl_seqno);
>> /* Cleans up the to be deleted timeout policy in the pending kill
>> list. */
>> void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_);
>> +
>> + /* Returns true if timeout policy for 'zone' is configured and
>> stores the
>> + * timeout policy id in '*tp_id'. */
>> + bool (*ct_zone_timeout_policy_get)(const struct ofproto *ofproto_,
>> + uint16_t zone, uint32_t *tp_id);
>> };
>>
>> extern const struct ofproto_class ofproto_dpif_class;
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 373b8a4eba0c..cef0690cf466 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -955,6 +955,17 @@ ofproto_ct_zone_timeout_policy_sweep(const struct
>> ofproto *ofproto)
>> }
>> }
>>
>> +bool
>> +ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto,
>> + uint16_t zone, uint32_t *tp_id)
>> +{
>> + if (ofproto->ofproto_class->ct_zone_timeout_policy_get) {
>> + return ofproto->ofproto_class->ct_zone_timeout_policy_get(
>> + ofproto, zone, tp_id);
>> + }
>> + return false;
>> +}
>> +
>>
>> /* Spanning Tree Protocol (STP) configuration. */
>>
>> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
>> index 2ae42374be36..8749cabdb7bd 100644
>> --- a/ofproto/ofproto.h
>> +++ b/ofproto/ofproto.h
>> @@ -366,6 +366,8 @@ void ofproto_set_vlan_limit(int vlan_limit);
>> void ofproto_ct_zone_timeout_policy_reconfig(const struct ofproto
>> *ofproto,
>> const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno);
>> void ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto);
>> +bool ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto,
>> + uint16_t zone, uint32_t *tp_id);
>>
>> /* Configuration of ports. */
>> void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
