> This is nice! The data structures are well documented, too--thanks for
> that!
>
> I get the following sparse warning:
>
> ../ofproto/ofproto-dpif-hsa.c:722:56: warning: incorrect type in
> argument 2 (different base types)
> ../ofproto/ofproto-dpif-hsa.c:722:56: expected restricted
> ofp_port_t [usertype] port
> ../ofproto/ofproto-dpif-hsa.c:722:56: got unsigned long long
> [unsigned] [usertype] port
>
>
Fixed,
> The copyright notices should probably list 2015 and not the other years.
>
> Because each element in move_map[] acts like an array of two elements,
> would it be worthwhile actually making it a 2-d array, like "uint8_t
> move_map[MOVE_MAP][2]"?
>
>
Yeah, that is more clear.
> I am not sure how to interpret this:
> * Assume all fields are in big endian.
>
>
This is to formalize the transition between field bit and the move_map
element. All related operations use the mf_value/mf_subfield which
represents the field as ovs_be type.
To make it more clear, I change the comment to:
* Assume all fields in 'struct flow' are in big endian. This is to
* formalize the transition between field bit and the move_map element.
> hs_clone() copies some large members (match_hs, match_src, move_map)
> that were just zeroed in the call to hs_create(). It might be worth
> avoiding the double initialization.
>
>
Yeah, I'll do that,
> I think that the use of list_insert() in the LIST_FOR_EACH loop in
> hs_clone() will make the clone's list of constraints be in the order
> opposite of the original header_space. That might be undesirable; you
> could use list_push_back() to avoid it.
>
>
I'll fix this thx! definitely undesirable but don't think this will make
the result
wrong.
> You could use xmemdup() in place of xmalloc() + memcpy(), in hs_clone().
>
>
Thx, will use it~
> In hsa_rule_compare(), the use of subtraction for comparison is risky if
> the priorities could be large enough to overflow. Currently we don't
> use priorities big enough for that to happen, but it's not nice to risk
> it.
>
> Here is one way to do it safely:
> return r2->prio > r1->prio ? -1 : r2->prio < r1->prio;
>
>
Thx for the suggestion,
> It seems inefficient in hs_constraint_is_exact_match() to initialize two
> flow_wildcards structures. I think you can use is_all_ones() and
> is_all_zeros() instead.
>
Absolutely,
>
> I don't think the list_is_empty() check in hsa_finish() or
> hsa_debug_dump_flows() is needed.
>
> I'm not fully comfortable with assert-failing (killing ovs-vswitchd) if
> the flow table has unsupported features in it. Also, even when HSA
> doesn't kill the process due to assert-failing, it could still delay the
> process by an arbitrary amount of time. Do you have an idea for a way
> to keep curious admins from accidentally taking down their switch by
> running an HSA command ("hey, what does this ovs-appctl command do?")?
>
>
Sure, the asserts are more for alerting things at development time.
Will change to VLOG.
Also, I can only think of separating the logic info a thread to prevent
it from blocking the main thread?
> I don't know why this is a CONST_CAST, in
> hsa_move_map_set_field_by_subfield():
> ((uint8_t *) &value)[0] = CONST_CAST(uint8_t, src->field->id);
>
> For initializing and examining a uint16_t that's divided into two
> byte-size fields, as in hsa_move_map_set_field_by_subfield() or
> hsa_move_map_apply_matched_field(), it's more usual to just use bitwise
> operators.
>
I see. Since I change the move_map to 2-D array, that will solve the issue
here.
> There's a lot of code here and I only really scratched the surface for
> understanding it.
>
>
Yeah, really grateful for the review~ ;D~
Hope it is some okay work to review~
> Is it possible to write some tests?
>
>
Yes, will add them to tests.
> Here are some suggestions as an incremental diff:
>
> diff --git a/ofproto/ofproto-dpif-hsa.c b/ofproto/ofproto-dpif-hsa.c
> index 7065c9b..38e6614 100644
> --- a/ofproto/ofproto-dpif-hsa.c
> +++ b/ofproto/ofproto-dpif-hsa.c
> @@ -114,7 +114,7 @@ struct header_space {
> * array[0], the latter value is at array[1].
> *
> * The value is 'BIT_UNSET' if the field has never been set, or
> - * 'BIT_SET' if the field has been set by aciton.
> + * 'BIT_SET' if the field has been set by action.
> * */
> uint16_t move_map[MOVE_MAP_LEN];
>
> @@ -217,7 +217,7 @@ hs_create(void)
>
> hs->output = OFPP_NONE;
> list_init(&hs->constraints);
> - memset(hs->move_map, 0xff, MOVE_MAP_LEN * sizeof(uint16_t));
> + memset(hs->move_map, 0xff, MOVE_MAP_LEN * sizeof *hs->move_map);
> hmap_init(&hs->matched_map);
>
> return hs;
> @@ -320,7 +320,7 @@ hsa_rule_swap(size_t a, size_t b, void *aux)
> list_swap(list_at_position(rules, a), list_at_position(rules, b));
> }
>
> -/* This compares is implemented for sorting in descending order. */
> +/* This comparison is implemented for sorting in descending order. */
> static int
> hsa_rule_compare(size_t a, size_t b, void *aux)
> {
> @@ -649,9 +649,9 @@ hsa_rule_apply_output_action__(struct header_space
> *hs, ofp_port_t port,
> case OFPP_LOCAL:
> case OFPP_NORMAL:
> /* Should not see such actions installed from controller. */
> - ovs_assert(false);
> + OVS_NOT_REACHED();
> case OFPP_CONTROLLER:
> - /* Do not thing. */
> + /* Do nothing. */
> break;
> default:
> if (port != hs->match_src.flow.in_port.ofp_port) {
> @@ -1165,7 +1165,7 @@ hsa_mf_are_prereqs_ok(const struct mf_field *mf,
> case MFP_ND:
> case MFP_ND_SOLICIT:
> case MFP_ND_ADVERT:
> - ovs_assert(false);
> + OVS_NOT_REACHED();
> }
> OVS_NOT_REACHED();
> }
> @@ -1531,7 +1531,7 @@ hsa_unixctl_leak_detect(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
> {
> struct ds out = DS_EMPTY_INITIALIZER;
>
> - op_type=HSA_LEAK_DETECT;
> + op_type = HSA_LEAK_DETECT;
> hsa_do_analysis(&out, argv[1], argv[2]);
> unixctl_command_reply(conn, ds_cstr(&out));
> ds_destroy(&out);
> @@ -1543,7 +1543,7 @@ hsa_unixctl_loop_detect(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
> {
> struct ds out = DS_EMPTY_INITIALIZER;
>
> - op_type=HSA_LOOP_DETECT;
> + op_type = HSA_LOOP_DETECT;
> hsa_do_analysis(&out, argv[1], argv[2]);
> unixctl_command_reply(conn, ds_cstr(&out));
> ds_destroy(&out);
>
>
Will resend the series after addressing all issues~
Thanks,
Alex Wang,
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
