[ovs-dev] [RFC HSA 4/4] ofprot-dpif-hsa: Implement HSA prototype.

Alex Wang alexw at nicira.com
Mon Jun 1 00:35:14 UTC 2015


> 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,



More information about the dev mailing list