Hi Eelco,
MFEX v7 will be available shorty EOD today.
Some comments are inline.
<Sniped>
> > + * for that packet.
> > + */
> > + uint32_t mfex_hit = (mf_mask & (1 << i));
>
> This was supposed to become a bool?
>
This cannot be a bool as this is used like a bit-mask and set bits are used to
iterate the packets.
> >
> > + /* Call probe on each impl, and cache the result. */
> > + uint32_t i;
> > + for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
>
> Why not use int here? Something like will also make Benn happy:
>
> for (int i = 0; i < ARRAY_SIZE(mfex_impls); i++)...
>
Taken into v7.
> > + bool avail = true;
> > + if (mfex_impls[i].probe) {
> > + /* Return zero is success, non-zero means error. */
> > + avail = (mfex_impls[i].probe() == 0);
> > + }
> > + VLOG_INFO("Miniflow Extract implementation %s (available: %s)\n",
> > + mfex_impls[i].name, avail ? "True" : "False");
> > + mfex_impls[i].available = avail;
> > + }
> > +}
> > +
> > +miniflow_extract_func
> > +dp_mfex_impl_get_default(void)
> > +{
> > + /* For the first call, this will be NULL. Compute the compile time
> > default.
> > + */
> > + if (!default_mfex_func) {
> > +
> Guess the empty newline can be removed.
>
Taken into v7.
> > + VLOG_INFO("Default MFEX implementation is %s.\n",
> > + mfex_impls[MFEX_IMPL_SCALAR].name);
>
> As the default validator has its function defined as NULL, the vlog will be
> called
> for each call to dp_mfex_impl_get_default().
> So I think this should become a VLOG_ONCE().
>
Yes using VLOG_INFO_ONCE().
> > + default_mfex_func =
> > + mfex_impls[MFEX_IMPL_SCALAR].extract_func;
>
> From the top of my head, setting and getting this value only happens from a
> single thread. Can you confirm? If not, this also needs to be atomically
> set/get.
>
Agreed taken into v7.
> > + }
> > +
> > + return default_mfex_func;
> > +}
> > +
> > +int32_t
>
> This should just be int, don’t see the use case for forcing this to 32-bit
> especially
> for error values?
>
Fixed.
> > +dp_mfex_impl_set_default_by_name(const char *name) {
> > + miniflow_extract_func new_default;
> > +
> > +
>
> Guess only one newline is needed.
>
Fixed.
> > + int32_t err = dp_mfex_impl_get_by_name(name, &new_default);
>
> int? See above. Also it’s only used in this function, so what about making it
> static?
>
> > +
> > + if (!err) {
> > + default_mfex_func = new_default;
>
> As we log the default implementation above, should we not also log changing
> it?
>
We can but we would be adding un-necessary log as set command already has logs
to indicate it.
> > + }
> > +
> > + return err;
> > +
> > +}
> > +
> > +uint32_t
>
> Maybe the return value should be size_t?
>
Fixed.
> > +dp_mfex_impl_get(struct ds *reply, struct dp_netdev_pmd_thread
> **pmd_list,
> > + size_t n)
>
> Can n be more specific? Like pmd_list_size?
>
Fixed.
> > +{
> > + /* Add all mfex functions to reply string. */
> > + ds_put_cstr(reply, "Available MFEX implementations:\n");
> > +
> > + for (uint32_t i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
>
> i can just be int here, so for (int i = 0; ...
>
Fixed.
> > +
> > + ds_put_format(reply, " %s (available: %s)(pmds: ",
> > + mfex_impls[i].name, mfex_impls[i].available ?
> > + "True" : "False");
> > +
> > + for (size_t j = 0; j < n; j++) {
> > + struct dp_netdev_pmd_thread *pmd = pmd_list[j];
> > + if (pmd->core_id == NON_PMD_CORE_ID) {
> > + continue;
> > + }
> > +
> > + if (pmd->miniflow_extract_opt == mfex_impls[i].extract_func) {
> > + ds_put_format(reply, "%u,", pmd->core_id);
> > + }
> > + }
> > +
> > + ds_chomp(reply, ',');
> > +
> > + if (ds_last(reply) == ' ') {
> > + ds_put_cstr(reply, "none");
> > + }
> > +
> > + ds_put_cstr(reply, ")\n");
> > + }
> > +
> > + return ARRAY_SIZE(mfex_impls);
> > +}
> > +
> > +/* This function checks all available MFEX implementations, and
> > +selects the
> > + * returns the function pointer to the one requested by "name".
> > + */
> > +int32_t
>
> See above, I would define this as int.
>
Fixed.
> > +dp_mfex_impl_get_by_name(const char *name, miniflow_extract_func
> > +*out_func) {
> > + if ((name == NULL) || (out_func == NULL)) {
> > + return -EINVAL;
> > + }
> > +
> > + uint32_t i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
>
> Think i, should be int, so:
>
> for (int i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
>
Fixed.
> > + if (strcmp(mfex_impls[i].name, name) == 0) {
> > + /* Probe function is optional - so check it is set before
> > exec. */
> > + if (!mfex_impls[i].available) {
> > + *out_func = NULL;
> > + return -EINVAL;
>
> Maybe we should return a different error, so we can distingue between the two?
> For example ENODEV?
>
Fixed.
> > + }
> > +
> > + *out_func = mfex_impls[i].extract_func;
> > + return 0;
> > + }
> > + }
> > +
> > + return -EINVAL;
>
> Maybe return -ENOENT, so we know the entry was not found?
>
Fixed.
*pmd_handle);
> > +
>
> Thought we would add a BUILD_ASSERT() to make sure the
> NETDEV_MAX_BURST does not exceed our uint32_t return code? Or did I miss it
> somewhere?
>
Assert added in v7.
> > +/* Probe function is used to detect if this CPU has the ISA required
> > + * to run the optimized miniflow implementation.
> > + * returns one on successful probe.
> > + * returns zero on failure.
> > + */
> > +typedef int32_t (*miniflow_extract_probe)(void);
>
> I think we can make this an int, see no need to cast it to 32-bit?
>
Agreed and fixed.
> > +
> > +/* This function returns all available implementations to the caller.
> > +The
> > + * quantity of implementations is returned by the int return value.
> > + */
> > +uint32_t
> > +dp_mfex_impl_get(struct ds *reply, struct dp_netdev_pmd_thread
> **pmd_list,
> > + size_t n);
>
> Thinks this needs the OVS_REQUIRES(dp_netdev_mutex) addition.
>
Added in v7.
> > +
> > +/* This function checks all available MFEX implementations, and
> > +selects the
> > + * returns the function pointer to the one requested by "name".
> > + */
> > +int32_t
void *aux OVS_UNUSED) {
> > + struct ds reply = DS_EMPTY_INITIALIZER;
> > + struct shash_node *node;
> > + uint32_t count = 0;
> > +
>
> We should hold the dp_netdev_mutex for the below…
>
Added a mutex .
> > + SHASH_FOR_EACH (node, &dp_netdevs) {
> > + struct dp_netdev *dp = node->data;
> > +
> > + /* Get PMD threads list. */
> > + size_t n;
> > + struct dp_netdev_pmd_thread **pmd_list;
> > + sorted_poll_thread_list(dp, &pmd_list, &n);
> > + count = dp_mfex_impl_get(&reply, pmd_list, n);
> > + }
> > +
> > + if (count == 0) {
> > + unixctl_command_reply_error(conn, "Error getting Mfex
> > + names.");
>
> In other log messages you use all capital MFEX, can we make it consistent?
>
Fixed.
> > + } else {
> > + unixctl_command_reply(conn, ds_cstr(&reply));
>
>
> Here log/output should be fixed as selecting a unavailable output shows wrong
> error message (see also previous comment):
>
Fixed the formatting .
> $ ovs-appctl dpif-netdev/miniflow-parser-set avx512_dot1q_ipv4_udp
> Miniflow implementation not available: Unknown miniflow implementation
> avx512_dot1q_ipv4_udp.
>
>
> > + ds_destroy(&reply);
> > + ovs_mutex_unlock(&dp_netdev_mutex);
>
> The unlock could as the first thing after the if(err) to save avoid holding
> it when
> doing slow logging etc. just like below.
>
Fixed .
> > + return;
> > + }
>
>
> Argument handling is not as it should be, see my previous comment. I think the
> packets count should only be available for the study option (this might not be
> the correct patch, but just want to make sure it’s addressed, and I do not
> forget).
>
> So as an example this looks odd trying to set it for a specific PMD:
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator 15 1
> Miniflow implementation set to autovalidator, on pmd thread 1
>
> Why do I have to put in the dummy value 15. Here is a quote from my previous
> comment:
>
> “
> We also might need to re-think the command to make sure
> packet_count_to_study is only needed for the study command.
> So the help text might become something like:
>
> dpif-netdev/miniflow-parser-set {miniflow_implementation_name | study
> [pkt_cnt]} [dp] [pmd_core] ”
>
>
See Harrys mail .
Keeping study_cnt at the end does provide easy use and more visibility
combining benefits of
your proposal and old implementation.
>
> > +
> > + SHASH_FOR_EACH (node, &dp_netdevs) {
> > + struct dp_netdev *dp = node->data;
> > +
> > + /* Get PMD threads list. */
> > + size_t n;
> > + struct dp_netdev_pmd_thread **pmd_list;
> > + sorted_poll_thread_list(dp, &pmd_list, &n);
> > +
> > + for (size_t i = 0; i < n; i++) {
> > + struct dp_netdev_pmd_thread *pmd = pmd_list[i];
> > + if (pmd->core_id == NON_PMD_CORE_ID) {
> > + continue;
> > + }
> > +
> > + /* Initialize MFEX function pointer to the newly configured
> > + * default. */
> > + miniflow_extract_func default_func =
> > + dp_mfex_impl_get_default();
>
> This can be done outside of the loop, so it only needs to be done once.
>
Taken outside.
> > + atomic_uintptr_t *pmd_func = (void *)
> > &pmd->miniflow_extract_opt;
> > + atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
> > + };
> > + }
> > +
> > + ovs_mutex_unlock(&dp_netdev_mutex);
> > +
> > + /* Reply with success to command. */
> > + struct ds reply = DS_EMPTY_INITIALIZER;
> > + ds_put_format(&reply, "Miniflow implementation set to %s.\n",
> > + mfex_name);
>
> In other places we use "Miniflow Extract implementation", so I think we should
> use the same here, i.e. “Miniflow Extract implementation set to %s.”. Even
> with
> this we still use MFEX and "Miniflow Extract", maybe we should consolidate to
> use either one? I think "Miniflow Extract" might be the best as MFEX might
> confuse users?
>
Using Miniflow extract implementation as standard.
> > + const char *reply_str = ds_cstr(&reply);
> > + unixctl_command_reply(conn, reply_str);
> > + VLOG_INFO("%s", reply_str);
> > + ds_destroy(&reply);
> > +}
> > +
> > static void
> > dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
> > const char *argv[], void *aux OVS_UNUSED)
> > @@ -1298,6 +1390,13 @@ dpif_netdev_init(void)
> > unixctl_command_register("dpif-netdev/dpif-impl-get", "",
> > 0, 0, dpif_netdev_impl_get,
> > NULL);
> > + unixctl_command_register("dpif-netdev/miniflow-parser-set",
> > + "miniflow implementation name",
>
> Not it looks like three individual arguments can be passed, guess it needs to
> become miniflow_implementation_name.
>
Fixed.
> > + 1, 1, dpif_miniflow_extract_impl_set,
> > + NULL);
> > --
> > 2.32.0
Regards
Amber
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
