[ovs-dev] [PATCH 07/12] dpif-netlink: Add conntrack timeout policy support
Yi-Hung Wei
yihung.wei at gmail.com
Mon Jul 29 19:46:14 UTC 2019
On Fri, Jul 26, 2019 at 10:15 AM William Tu <u9012063 at gmail.com> wrote:
> > +static void
> > +dpif_netlink_format_tp_name(uint32_t id, uint16_t l3num, uint8_t l4num,
> > + struct ds *tp_name)
> > +{
> > + ds_clear(tp_name);
> > + ds_put_format(tp_name, "%s%"PRIu32"_", NL_TP_NAME_PREFIX, id);
> > + ct_dpif_format_ipproto(tp_name, l4num);
> > +
> > + if (l3num == AF_INET) {
> > + ds_put_cstr(tp_name, "4");
> > + } else if (l3num == AF_INET6 && l4num != IPPROTO_ICMPV6) {
>
> Why excluding IPPROTO_ICMPV6 above?
Thanks for review.
It is because ct_dpif_format_ipproto returns "icmpv6" for
IPPROTO_ICMPV6 and "icmp" for "IPPROTO_ICMP", and I found it to be
confusing to have ovs_tp_<tp_id>_icmpv66 as the timeout policy name.
> > +#define CT_DPIF_TO_NL_TP_MAPPING(PROTO1, PROTO2, ATTR1, ATTR2) \
> > +if (nl_tp->present & (1 << CTA_TIMEOUT_##PROTO2##_##ATTR2)) { \
> > + tp->present |= 1 << CT_DPIF_TP_ATTR_##PROTO1##_##ATTR1; \
> > + tp->attrs[CT_DPIF_TP_ATTR_##PROTO1##_##ATTR1] = \
> > + nl_tp->attrs[CTA_TIMEOUT_##PROTO2##_##ATTR2]; \
> > + }
> > +
> > +static void
> > +dpif_netlink_set_ct_dpif_tp_tcp_attrs(const struct nl_ct_timeout_policy *nl_tp,
> > + struct ct_dpif_timeout_policy *tp)
> > +{
> > + CT_DPIF_TO_NL_TP_TCP_MAPPINGS
>
> Is this better to renamed as CT_DPIF_FROM_NL_TP_TCP_MAPPINGS?
>
> You're using the same macro name, one for
> setting the nl_tp->attrs from tp->attrs, the other for
> setting the tp->attrs from nl_tp_attrs
Thanks for the suggestion. As our offline discussion, it is confusing
to have "_TO_" in the marco name, I will get rid of it.
> > +static int
> > +dpif_netlink_ct_add_timeout_policy(struct dpif *dpif OVS_UNUSED,
> > + bool is_default,
> > + const struct ct_dpif_timeout_policy *tp)
> > +{
> > +#ifdef _WIN32
> > + return EOPNOTSUPP;
> > +#else
> > + struct nl_ct_timeout_policy nl_tp;
> > + struct ds ds = DS_EMPTY_INITIALIZER;
> > + int i, err;
> > +
> > + for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
> > + dpif_netlink_format_tp_name(tp->id, tp_protos[i].l3num,
> > + tp_protos[i].l4num, &ds);
> > + ovs_strlcpy(nl_tp.name, ds_cstr(&ds), sizeof nl_tp.name);
> > + nl_tp.l3num = tp_protos[i].l3num;
> > + nl_tp.l4num = tp_protos[i].l4num;
> > + dpif_netlink_get_nl_tp_attrs(tp, tp_protos[i].l4num, &nl_tp);
> > + if (!is_default) {
> > + err = nl_ct_set_timeout_policy(&nl_tp);
> > + } else if (tp_protos[i].l3num == AF_INET) {
> > + /* The default timeout policy is shared between AF_INET and
> > + * AF_INET6 in the kernel. So configure AF_INET is sufficient. */
> > + err = nl_ct_set_default_timeout_policy(&nl_tp);
> > + }
> > + if (err) {
> > + VLOG_INFO("failed to set timeout policy %s (%s)", nl_tp.name,
> > + ovs_strerror(err));
> ds_destroy(&ds);
Thanks, I will destroy the dynamic string properly in all the
following cases in v2.
> > +static int
> > +dpif_netlink_ct_get_timeout_policy(struct dpif *dpif OVS_UNUSED,
> > + bool is_default, uint32_t tp_id,
> > + struct ct_dpif_timeout_policy *tp)
> > +{
> > +#ifdef _WIN32
> > + return EOPNOTSUPP;
> > +#else
> if _WIN32 is alway return EOPNOTSUPP,
> is it better if we aggregate all 6 functions and have a larger
> #ifdef _WIN32
> // all six functions return EOPNOTSUPP
> #else
> // actual implementations
> #endif
Sure, I will make proper change to make the code looks clearly in the
next version.
> > + struct nl_ct_timeout_policy nl_tp;
> > + struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
> > + int i, err;
> > +
> > + tp->id = tp_id;
> > + tp->present = 0;
> > + for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
> > + if (!is_default) {
> > + dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
> > + tp_protos[i].l4num, &nl_tp_name);
> > + err = nl_ct_get_timeout_policy(ds_cstr(&nl_tp_name), &nl_tp);
> > + } else if (tp_protos[i].l3num == AF_INET) {
> > + /* The default timeout is shared between AF_INET and AF_INET6
> > + * in the kernel. So get from AF_INET is sufficient. */
> Then why checking 'tp_protos[i].l3num == AF_INET'?
> What happens when tp_protos[i].l3num == AF_INET6? then 'err' becomes uninitialized.
This function is called from ct-dpif to query the timeout policy
stored in the kernel. It will loop through all L3/L4 pairs (ipv4
tcp/udp/icmp and ipv6 tcp/udp/icmpv6). The main purpose for this
check is to skip AF_INET6 cases for default timeout since it does not
distingush the ipv4 and ipv6 cases in the kernel.
> > +static int
> > +dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED,
> > + uint32_t tp_id)
> > +{
> > +#ifdef _WIN32
> > + return EOPNOTSUPP;
> > +#else
> > + struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
> > + int i, err;
> > +
> > + if (!tp_id) {
> > + return EINVAL;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
> > + dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
> > + tp_protos[i].l4num, &nl_tp_name);
> > + err = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
> > + if (err) {
> > + VLOG_INFO("failed to delete timeout policy %s (%s)",
> > + ds_cstr(&nl_tp_name), ovs_strerror(err));
> Use VLOG_WARN? or VLOG_WARN_RL?
I will change that to VLOG_WARN_RL in v2.
> > +static int
> > +dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
> > + void *state,
> > + struct ct_dpif_timeout_policy **tp)
> > +{
> > +#ifdef _WIN32
> > + return EOPNOTSUPP;
> > +#else
> > + struct dpif_netlink_ct_timeout_policy_dump_state *dump_state = state;
> > + struct dpif_netlink_tp_dump_node *tp_dump_node;
> > + struct nl_ct_timeout_policy nl_tp;
> > + uint32_t tp_id;
> > + int err;
> > +
> > + do {
> > + err = nl_ct_timeout_policy_dump_next(dump_state->nl_dump_state,
> > + &nl_tp);
> > + if (err) {
> > + break;
> > + }
> > +
> > + if (!ovs_scan(nl_tp.name, NL_TP_NAME_PREFIX"%"PRIu32, &tp_id)) {
> > + continue;
> > + }
> > +
> > + tp_dump_node = get_dpif_netlink_tp_dump_node_by_tp_id(
> > + tp_id, &dump_state->tp_dump_map);
> > + if (!tp_dump_node) {
> > + tp_dump_node = xzalloc(sizeof *tp_dump_node);
> > + tp_dump_node->tp = xzalloc(sizeof *tp_dump_node->tp);
> > + tp_dump_node->tp->id = tp_id;
> > + hmap_insert(&dump_state->tp_dump_map, &tp_dump_node->hmap_node,
> > + hash_int(tp_id, 0));
> > + }
> > +
> > + update_dpif_netlink_tp_dump_node(&nl_tp, tp_dump_node);
> > + if (tp_dump_node->present == DPIF_NL_ALL_TP) {
> > + hmap_remove(&dump_state->tp_dump_map, &tp_dump_node->hmap_node);
> > + *tp = tp_dump_node->tp;
> > + free(tp_dump_node);
> Do we have to remove and free tp_dump_node here?
> Isn't it done at dpif_netlink_ct_timeout_policy_dump_done()?
This is the case where we gather all of the 6 sub timeout policies and
return that to ct-dpif layer. Once a full profile is gathered, we will
report that to ct-dpif layer and reomve the tp_dump_node. What we
free in dpif_netlink_ct_timeout_policy_dump_done() is the incomplete
timeout policies.
Thanks,
-Yi-Hung
More information about the dev
mailing list