[ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

Darrell Ball dlu998 at gmail.com
Mon Jul 29 20:12:17 UTC 2019


On Mon, Jul 29, 2019 at 12:37 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

> On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu998 at gmail.com> wrote:
> >
> > Thanks for the patch
> >
> > I found this patch hard to review since it does not contain
> implementations
> > The same comment applies to Patch 6
> > I think Patches 5-7 can be combined into one patch, which will make
> review easier.
>
> Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in
> v2.
>
>
> >> +struct ct_dpif_timeout_policy {
> >> +    uint32_t    id;         /* id that uniquely identify a timeout
> policy. */
> >> +    uint32_t    present;    /* If a timeout attribute is present set
> the
> >> +                             * corresponding bit. */
> >>
> >> +    uint32_t    attrs[CT_DPIF_TP_ATTR_MAX];     /* An array that
> specifies
> >> +                                                 * timeout attribute
> values */
> >
> >
> > I think you can make attrs of type 'int32_t' and use '-1' timeout for
> 'not present' and then
> > remove the 'present' field
>
> The timeout value is uint32_t in the kernel, so I will keep it as
> uint32_t.  I find the present flag to be handy when doing conversion
> from ct-dpif layer to dpif-netlink layer, and as you mentioned in the
> following e-mail, I will keep it as is.
>
>
> >> --- a/lib/dpif-netlink.c
> >> +++ b/lib/dpif-netlink.c
> >> @@ -3434,6 +3434,12 @@ const struct dpif_class dpif_netlink_class = {
> >>      dpif_netlink_ct_set_limits,
> >>      dpif_netlink_ct_get_limits,
> >>      dpif_netlink_ct_del_limits,
> >> +    NULL,                       /* ct_set_timeout_policy */
> >> +    NULL,                       /* ct_get_timeout_policy */
> >> +    NULL,                       /* ct_del_timeout_policy */
> >> +    NULL,                       /* ct_timeout_policy_dump_start */
> >> +    NULL,                       /* ct_timeout_policy_dump_next */
> >> +    NULL,                       /* ct_timeout_policy_dump_done */
> >
> >
> > I found this patch hard to review since it does not contain
> implementations
> > The same comment applies to Patch 6
> > I think Patches 5-7 can be combined into one patch, which will make
> review easier.
> >
> >
> >>
> >>      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 12898b9e3c6d..3460ef8aa98d 100644
> >> --- a/lib/dpif-provider.h
> >> +++ b/lib/dpif-provider.h
> >> @@ -80,6 +80,7 @@ dpif_flow_dump_thread_init(struct
> dpif_flow_dump_thread *thread,
> >>  struct ct_dpif_dump_state;
> >>  struct ct_dpif_entry;
> >>  struct ct_dpif_tuple;
> >> +struct ct_dpif_timeout_policy;
> >>
> >>  /* 'dpif_ipf_proto_status' and 'dpif_ipf_status' are presently in
> >>   * sync with 'ipf_proto_status' and 'ipf_status', but more
> >> @@ -498,6 +499,48 @@ struct dpif_class {
> >>       * list of 'struct ct_dpif_zone_limit' entries. */
> >>      int (*ct_del_limits)(struct dpif *, const struct ovs_list
> *zone_limits);
> >>
> >> +    /* Connection tracking timeout policy */
> >> +
> >> +    /* A connection tracking timeout policy contains a list of timeout
> >> +     * attributes that specifies timeout values on various connection
> states.
> >> +     * In a datapath, the timeout policy is identified by a 4 bytes
> unsigned
> >> +     * integer, and the unsupported timeout attributes are ignored.
> >> +     * When a connection is committed it can be associated with a
> timeout
> >> +     * policy, or it defaults to the default timeout policy. */
> >> +
> >> +    /* Add timeout policy '*tp' into the datapath.  If 'is_default' is
> true
> >
> >
> > "is_default" - can you explain this one ?
>
> This flag is used to configure the default timeout policy in the
> datapath.  If 'is_default' is true, it will set the provided timeout
> policy to be the default timeout policy. The default timeout policy is
> documented in vswitchd/vswitch.xml
>
>
Below is what I see the the schema xml file under timeout policy
Please add description about the 'default' timeout policy under
"CT_Timeout_Policy" - let me know if you need help, as I can submit a patch.

  <table name="CT_Timeout_Policy">
    Connection tracking timeout policy configuration

    <group title="Timeouts">
      <column name="timeouts">
          The <code>timeouts</code> column contains key-value pairs used
          to configure connection tracking timeouts in a datapath.
          Key-value pairs that are not supported by a datapath are
          ignored.
      </column>

      <group title="TCP Timeouts">
        <column name="timeouts" key="tcp_syn_sent">
          TCP SYN sent timeout.
        </column>

        <column name="timeouts" key="tcp_syn_recv">
          TCP SYN receive timeout.
        </column>

        <column name="timeouts" key="tcp_established">
          TCP established timeout.
        </column>

        <column name="timeouts" key="tcp_fin_wait">
          TCP FIN wait timeout.
        </column>

        <column name="timeouts" key="tcp_close_wait">
          TCP close wait timeout.
        </column>

        <column name="timeouts" key="tcp_last_ack">
          TCP last ACK timeout.
        </column>

        <column name="timeouts" key="tcp_time_wait">
          TCP time wait timeout.
        </column>

        <column name="timeouts" key="tcp_close">
          TCP close timeout.
        </column>

        <column name="timeouts" key="tcp_syn_sent2">
          TCP syn sent2 timeout.
        </column>

        <column name="timeouts" key="tcp_retransmit">
          TCP retransmit timeout.
        </column>

        <column name="timeouts" key="tcp_unack">
          TCP unacknowledgment timeout.
        </column>
      </group>

      <group title="UDP Timeouts">
        <column name="timeouts" key="udp_first">
          First UDP packet timeout.
        </column>

        <column name="timeouts" key="udp_single">
          The timeout in the state that source host sends more than one
packet
          but the destination host has never sent one backs.
        </column>

        <column name="timeouts" key="udp_multiple">
          UDP packets seen in both directions timeout.
        </column>
      </group>

      <group title="ICMP Timeouts">
        <column name="timeouts" key="icmp_first">
          First ICMP timeout.
        </column>

        <column name="timeouts" key="icmp_reply">
          ICMP reply timeout.
        </column>
      </group>
    </group>





> >>
> >> +     * make the timeout policy to be the default timeout policy. */
> >> +    int (*ct_add_timeout_policy)(struct dpif *, bool is_default,
> >> +                                 const struct ct_dpif_timeout_policy
> *tp);
> >> +    /* Gets a timeout policy and stores that into '*tp'.
> >
> >
> >
> >>
> >>   If 'is_default' is
> >> +     * true, sets '*tp' to the default timeout policy.  Otherwise,
> gets the
> >
> >
> > The above text does not make sense:
> >  "sets" ?
> >
> > "is_default" - can you explain this one ?
>
> I re-wreite the comment as following.
>     /* Gets a timeout policy and stores that into '*tp'.  If 'is_default'
> is
>      * true, gets default timeout policy.  Otherwise, gets the timeout
>      * policy identified by 'tp_id'. */
>     int (*ct_get_timeout_policy)(struct dpif *, bool is_default,
>                                  uint32_t tp_id,
>                                  struct ct_dpif_timeout_policy *tp);
>
> Thanks,
>
> -Yi-Hung
>


More information about the dev mailing list