On Tue, Aug 13, 2019 at 7:46 PM Darrell Ball <dlu...@gmail.com> wrote:
>
> Thanks for the patch
>
> Some high level comments:
>
> 1/ The ct_tp_kill_list code is still in common code
> I think we discussed moving that to the dpif backer code
> ct_timeout_policy_unref() is adding to this deferred kill list which is
> not needed for userspace
> datapath.
> 2/ clear_existing_ct_timeout_policies() is in common code, but only does
> something if
> ct_dpif_timeout_policy_dump_start/next/done are realized in the datatype type
> specific code
> (which is only for the kernel code, which is correct). I think it would be
> cleaner and less confusing
> just to make the API clear_existing_ct_timeout_policies() kernel specific;
> i.e. in dpif-netlink.
Thanks for review. I address most of the code changes as in the
detailed inline code review.
For the two high level concerns, it is mainly because currently
ct_tp_kill_list is maintained in ofproto-dpif.c I thought about to
move the ct_tp_kill_list implementation from dpif_backer (in
ofproto-dpif.c) to dpif-netlink.c, and here is why I still keep it in
ofproto-dpif.c in the dpif_backer layer in this version.
AFAIK, currently, we do not have a proper place to store
ct_tp_kill_list in dpif-netlink.c in the userspace. dpif-netlink is
for the kernel datapath implementation, all the information that we
configured to dpif-netlink are directly pass down into the kernel
currently. In userspace datapath, we can store userspace specific
information in "struct dp_netdev", but there is no such place in
dpif-neltink for now. In this case, it is naturally to maintain the
ct_tp_kill_list one level up in the dpif_backer layer.
Anyhow, we can always make proper change on the way we maintain
timeout policy in ofproto-dpif layer when the dpif-netdev
implementation is introduced.
> On Mon, Aug 12, 2019 at 5:55 PM Yi-Hung Wei <yihung....@gmail.com> wrote:
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 751535249e21..3013d83e96a0 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -694,6 +718,8 @@ struct odp_garbage {
>>
>> static void check_support(struct dpif_backer *backer);
>>
>> +#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
>
>
> seems like random placement; could be moved where it is used.
>
ok, I will move it right before ct_zone_config_init().
>> +static struct ct_timeout_policy *
>> +ct_timeout_policy_alloc__(void)
>> +{
>> + struct ct_timeout_policy *ct_tp = xzalloc(sizeof *ct_tp);
>> + simap_init(&ct_tp->tp);
>> + return ct_tp;
>> +}
>
>
> by using above API, you are not saving any code and maybe more error prone
>
This function is used in ct_timeout_policy_alloc() and
clear_existing_ct_timeout_policies(). So are you sugguesting to expand
it in these two functions?
>>
>> +
>> +static struct ct_timeout_policy *
>> +ct_timeout_policy_alloc(struct simap *tp, struct id_pool *tp_ids)
>> +{
>> + struct simap_node *node;
>> +
>> + struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
>> + SIMAP_FOR_EACH (node, tp) {
>> + simap_put(&ct_tp->tp, node->name, node->data);
>> + }
>> +
>> + if (!id_pool_alloc_id(tp_ids, &ct_tp->tp_id)) {
>> + VLOG_ERR_RL(&rl, "failed to allocate timeout policy id.");
>> + simap_destroy(&ct_tp->tp);
>> + free(tp);
>
>
> I think you rather need to free 'ct_tp'; i.e. free(ct_tp)
Yes, thanks for spotting this bug.
>> +static void
>> +clear_existing_ct_timeout_policies(struct dpif_backer *backer)
>> +{
>> + /* In kernel datapath, when OVS starts, there may be some pre-existing
>> + * timeout policies in the kernel. To avoid reassign the same timeout
>> + * policy ids, we dump all the pre-existing timeout policies and keep
>> + * the ids in the pool. Since OVS will not use those timeout policies
>> + * for new datapath flow, we add them to the kill list and remove
>> + * them later on. */
>> + void *state;
>> +
>> + int err = ct_dpif_timeout_policy_dump_start(backer->dpif, &state);
>> + if (err) {
>> + return;
>> + }
>
>
> can be
>
> + if (ct_dpif_timeout_policy_dump_start(backer->dpif, &state)) {
> + return;
> + }
>
> similarly below
Sure, I will get rid of 'int err' in v4.
>> +
>> + struct ct_dpif_timeout_policy cdtp;
>> + while (!(err = ct_dpif_timeout_policy_dump_next(backer->dpif, state,
>> + &cdtp))) {
>> + struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
>> + ct_tp->tp_id = cdtp.id;
>> + id_pool_add(backer->tp_ids, cdtp.id);
>> + ovs_list_insert(&backer->ct_tp_kill_list, &ct_tp->list_node);
>
>
> not sure why you add to beginning here rather than end; was there some deeper
> meaning at play ?
Not really, I am fine to add it on either side tho.
>> +static void
>> +ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone)
>> +{
>> + struct dpif_backer *backer;
>> + struct ct_zone *ct_zone;
>> +
>> + backer = shash_find_data(&all_dpif_backers, datapath_type);
>
>
> struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> datapath_type);
Sure, will do in v4.
>>
>> + if (!backer) {
>> + return;
>> + }
>> +
>> + ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
>
>
> struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
Sure, added to v4.
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -935,6 +935,36 @@ ofproto_get_flow_restore_wait(void)
>> return flow_restore_wait;
>> }
>>
>> +/* Connection tracking configuration. */
>> +void
>> +ofproto_ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone,
>> + struct simap *timeout_policy)
>> +{
>> + const struct ofproto_class *class;
>> +
>> + datapath_type = ofproto_normalize_type(datapath_type);
>> + class = ofproto_class_find__(datapath_type);
>
>
> const struct ofproto_class *class = ofproto_class_find__(datapath_type);
Done in v4.
>> +void
>> +ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone)
>> +{
>> + const struct ofproto_class *class;
>> +
>> + datapath_type = ofproto_normalize_type(datapath_type);
>> + class = ofproto_class_find__(datapath_type);
>
> const struct ofproto_class *class = ofproto_class_find__(datapath_type);
Ack.
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -153,9 +153,35 @@ struct aa_mapping {
>> char *br_name;
>> };
>>
>> +/* Internal representation of conntrak zone configuration table in OVSDB. */
>
>
> 'conntrack'
Done.
>> +static struct datapath *
>> +datapath_create(const struct ovsrec_datapath *dp_cfg, const char *type)
>> +{
>> + struct datapath *dp;
>> +
>> + ovs_assert(!datapath_lookup(type));
>
> The caller just did a lookup before calling here; i.e you can remove assert.
Sounds good.
>>
>> + dp = xzalloc(sizeof *dp);
> struct datapath *dp = xzalloc(sizeof *dp);
>
ok.
>> +static void
>> +datapath_destroy(struct datapath *dp)
>> +{
>> + struct ct_zone *ct_zone;
>> +
>> + if (dp) {
>> + HMAP_FOR_EACH (ct_zone, node, &dp->ct_zones) {
>
> HMAP_FOR_EACH_SAFE
>
I replace HMAP_FOR_EACH to HMAP_FOR_EACH_SAFE in v3 for the following
3 cases that you mentioned.
Thanks,
-Yi-Hung
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
