[ovs-dev] [v11 06/11] dpif-netdev: Add packet count and core id paramters for study
Eelco Chaudron
echaudro at redhat.com
Wed Jul 14 10:50:43 UTC 2021
On 14 Jul 2021, at 12:30, Eelco Chaudron wrote:
> On 14 Jul 2021, at 4:02, kumar Amber wrote:
>
>> From: Kumar Amber <kumar.amber at intel.com>
>>
>> This commit introduces additional command line paramter
>> for mfex study function. If user provides additional packet out
>> it is used in study to compare minimum packets which must be processed
>> else a default value is choosen.
>> Also introduces a third paramter for choosing a particular pmd core.
>>
>> $ ovs-appctl dpif-netdev/miniflow-parser-set study 500 3
>>
>> Signed-off-by: Kumar Amber <kumar.amber at intel.com>
One additional comment, please add some (negative) test cases for the command line options, so we know your changes work. Rather than me having to do this manually every revision.
>> ---
>> v11:
>> - include comments from Eelco
>> - reworked set command as per discussion
>> v10:
>> - fix review comments Eelco
>> v9:
>> - fix review comments Flavio
>> v7:
>> - change the command paramters for core_id and study_pkt_cnt
>> v5:
>> - fix review comments(Ian, Flavio, Eelco)
>> - introucde pmd core id parameter
>> ---
>> ---
>> Documentation/topics/dpdk/bridge.rst | 37 ++++++-
>> lib/dpif-netdev-extract-study.c | 28 ++++-
>> lib/dpif-netdev-private-extract.h | 9 ++
>> lib/dpif-netdev.c | 156 ++++++++++++++++++++++-----
>> 4 files changed, 201 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst
>> index a47153495..7860d6173 100644
>> --- a/Documentation/topics/dpdk/bridge.rst
>> +++ b/Documentation/topics/dpdk/bridge.rst
>> @@ -284,12 +284,45 @@ command also shows whether the CPU supports each implementation ::
>>
>> An implementation can be selected manually by the following command ::
>>
>> - $ ovs-appctl dpif-netdev/miniflow-parser-set study
>> + $ ovs-appctl dpif-netdev/miniflow-parser-set [-pmd core_id] [name]
>> + [study_cnt]
>> +
>> +The above command has two optional parameters: study_cnt and core_id.
>> +The core_id sets a particular miniflow extract function to a specific
>> +pmd thread on the core.The third parameter study_cnt, which is specific
>
> Add space after period.
>
>> +to study and ignored by other implementations, means how many packets
>> +are needed to choose the best implementation.
>>
>> Also user can select the study implementation which studies the traffic for
>> a specific number of packets by applying all available implementations of
>> miniflow extract and then chooses the one with the most optimal result for
>> -that traffic pattern.
>> +that traffic pattern. The user can optionally provide an packet count
>> +[study_cnt] parameter which is the minimum number of packets that OVS must
>> +study before choosing an optimal implementation. If no packet count is
>> +provided, then the default value, 128 is chosen. Also, as there is no
>> +synchronization point between threads, one PMD thread might still be running
>> +a previous round, and can now decide on earlier data.
>> +
>> +The per packet count is a global value, and parallel study executions with
>> +differing packet counts will use the most recent count value provided by usser.
>
> usser -> user
>
>> +
>> +Study can be selected with packet count by the following command ::
>> +
>> + $ ovs-appctl dpif-netdev/miniflow-parser-set study 1024
>> +
>> +Study can be selected with packet count and explicit PMD selection
>> +by the following command ::
>> +
>> + $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 study 1024
>> +
>> +In the above command the last parameter is the CORE ID of the PMD
>
> This needs a re-write as this is no longer the last parameter.
>
>> +thread and this can also be used to explicitly set the miniflow
>> +extraction function pointer on different PMD threads.
>
>> +Scalar can be selected on core 3 by the following command where
>> +study count can be put as any arbitrary number or left blank ::
>
> This is also no longer correct, i.e., study count must NOT be provided.
>
>> +
>> + $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar
>>
>> Miniflow Extract Validation
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> diff --git a/lib/dpif-netdev-extract-study.c b/lib/dpif-netdev-extract-study.c
>> index 02b709f8b..0ed31aa46 100644
>> --- a/lib/dpif-netdev-extract-study.c
>> +++ b/lib/dpif-netdev-extract-study.c
>> @@ -25,7 +25,7 @@
>>
>> VLOG_DEFINE_THIS_MODULE(dpif_mfex_extract_study);
>>
>> -static atomic_uint32_t mfex_study_pkts_count = 0;
>> +static atomic_uint32_t mfex_study_pkts_count = MFEX_MAX_PKT_COUNT;
>
> This extra space is still present, see previous review.
>
>>
>> /* Struct to hold miniflow study stats. */
>> struct study_stats {
>> @@ -48,6 +48,27 @@ mfex_study_get_study_stats_ptr(void)
>> return stats;
>> }
>>
>> +int
>> +mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, const char *name)
>> +{
>> + struct dpif_miniflow_extract_impl *miniflow_funcs;
>> + miniflow_funcs = dpif_mfex_impl_info_get();
>> +
>> + /* If the packet count is set and implementation called is study then
>> + * set packet counter to requested number else set the packet counter
>> + * to default number.
>
> Also see previous review comment!
>
> “””
> Guess this comment is not correct, it’s just not set.
> “””
>
>> + */
>> + if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, name) == 0) &&
>> + (pkt_cmp_count != 0)) {
>> +
>> + atomic_uintptr_t *study_pck_cnt = (void *)&mfex_study_pkts_count;
>> + atomic_store_relaxed(study_pck_cnt, (uintptr_t) pkt_cmp_count );
>> + return 0;
>
> This is an atomic_uint32_t value, why store it as a uintptr?
> This will do:
>
> if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, name) == 0) &&
> (pkt_cmp_count != 0)) {
>
> - atomic_uintptr_t *study_pck_cnt = (void *)&mfex_study_pkts_count;
> - atomic_store_relaxed(study_pck_cnt, (uintptr_t) pkt_cmp_count );
> + atomic_store_relaxed(&mfex_study_pkts_count, pkt_cmp_count);
> return 0;
> }
>
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> uint32_t
>> mfex_study_traffic(struct dp_packet_batch *packets,
>> struct netdev_flow_key *keys,
>> @@ -86,7 +107,10 @@ mfex_study_traffic(struct dp_packet_batch *packets,
>> /* Choose the best implementation after a minimum packets have been
>> * processed.
>> */
>> - if (stats->pkt_count >= MFEX_MAX_PKT_COUNT) {
>> + static uint32_t study_cnt_pkts;
>> + atomic_read_relaxed(&mfex_study_pkts_count, &study_cnt_pkts);
>> +
>> + if (stats->pkt_count >= mfex_study_pkts_count) {
>
> You should use the read value here, study_cnt_pkts
>
>> uint32_t best_func_index = MFEX_IMPL_START_IDX;
>> uint32_t max_hits = 0;
>> for (int i = MFEX_IMPL_START_IDX; i < MFEX_IMPL_MAX; i++) {
>> diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-extract.h
>> index 9c03a1aa0..b93fecbbc 100644
>> --- a/lib/dpif-netdev-private-extract.h
>> +++ b/lib/dpif-netdev-private-extract.h
>> @@ -151,4 +151,13 @@ mfex_study_traffic(struct dp_packet_batch *packets,
>> uint32_t keys_size, odp_port_t in_port,
>> struct dp_netdev_pmd_thread *pmd_handle);
>>
>> +/* Sets the packet count from user to the stats for use in
>> + * study function to match against the classified packets to choose
>> + * the optimal implementation.
>> + * On error, returns -EINVAL.
>> + * On success, returns 0.
>> + */
>> +int
>> +mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, const char *name);
>> +
>> #endif /* MFEX_AVX512_EXTRACT */
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 707bf85c0..12c38a5a4 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -1076,34 +1076,87 @@ dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> }
>>
>> static void
>> -dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> +dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
>> const char *argv[], void *aux OVS_UNUSED)
>> {
>> - /* This function requires just one parameter, the miniflow name.
>> + /* A First optional paramter PMD thread ID can be also provided which
>> + * allows users to set miniflow implementation on a particular pmd.
>> + * The second paramter is the name of the function and if -pmd core_id
>> + * is not provided this is the first parameter and only mandatory one.
>> + * The third and last one is the study-cnt which is only provided for
>> + * study function to set the packet count.
>> */
>> - const char *mfex_name = argv[1];
>> + const char *mfex_name = NULL;
>> struct shash_node *node;
>> + int err = 0;
>> + bool pmd_core_param_set = false;
>> + struct ds reply = DS_EMPTY_INITIALIZER;
>> + const char *reply_str = NULL;
>
> As Harry already mentioned, this code is hard to follow and needs a re-write/cleanup.
>
> I do not agree that this should be a separate patch, as all changes are concentrate in this one. So please do this as part of v12 as you need it anyway.
>
> I will also reply to his email.
>
>> - int err = dp_mfex_impl_set_default_by_name(mfex_name);
>> + if ((strcmp("-pmd", argv[1]) == 0) && (argc >= 4)) {
>> + mfex_name = argv[3];
>> + pmd_core_param_set = true;
>>
>> - if (err) {
>> - struct ds reply = DS_EMPTY_INITIALIZER;
>> - char *error_desc = NULL;
>> - if (err == -EINVAL) {
>> - error_desc = "Unknown miniflow extract implementation:";
>> - } else if (err == -ENOENT) {
>> - error_desc = "Miniflow extract implementation doesn't exist:";
>> - } else if (err == -ENODEV) {
>> - error_desc = "Miniflow extract implementation not available:";
>> + } else if ((strcmp("-pmd", argv[1]) == 0) && (argc < 3)) {
>> + ds_put_format(&reply,
>> + "Error: Invalid PMD core_id or Null input");
>> + goto error;
>> +
>> + } else if (argc >= 2) {
>> + mfex_name = argv[1];
>> + }
>> +
>> + bool pmd_thread_specified = false;
>> + uint32_t pmd_thread_to_change = 0;
>> + bool pmd_thread_update_ok = false;
>> +
>> + if (pmd_core_param_set) {
>> + if (str_to_uint(argv[2], 10, &pmd_thread_to_change)) {
>> + pmd_thread_specified = true;
>> } else {
>> - error_desc = "Miniflow extract implementation Error:";
>> + /* argv[2] isn't even a uint. return without changing anything. */
>> + ds_put_format(&reply,
>> + "Error: Miniflow parser not changed, PMD thread argument"
>> + " passed is not valid: '%d'. Pass a valid pmd thread ID.\n",
>> + pmd_thread_to_change);
>> + goto error;
>> }
>> - ds_put_format(&reply, "%s %s.\n", error_desc, mfex_name);
>> - const char *reply_str = ds_cstr(&reply);
>> - unixctl_command_reply_error(conn, reply_str);
>> - VLOG_INFO("%s", reply_str);
>> - ds_destroy(&reply);
>> - return;
>> + }
>> +
>> + /* argv[4]/arg[2] is optional packet count, which user can provide along
>> + * with study function to set the minimum packet that must be matched in
>> + * order to choose the optimal function.
>> + */
>> + uint32_t study_ret = 0;
>> + uint32_t pkt_cmp_count = MFEX_MAX_PKT_COUNT;
>> +
>> + const char *study_cnt_param = NULL;
>> + if (argc == 5) {
>> + study_cnt_param = argv[4];
>> + } else if ((!pmd_core_param_set) && (argc == 3)) {
>> + study_cnt_param = argv[2];
>> + }
>> +
>> + if (study_cnt_param) {
>> +
>> + if (str_to_uint(study_cnt_param, 10, &pkt_cmp_count)) {
>> + study_ret = mfex_set_study_pkt_cnt(pkt_cmp_count, mfex_name);
>> + if (study_ret == -EINVAL) {
>> + ds_put_format(&reply, "The study_pkt_cnt option is not valid"
>> + " for the %s implementation.\n",
>> + mfex_name);
>> + goto error;
>> + }
>> + } else {
>> + ds_put_format(&reply, "Invalid study_pkt_cnt value: %s.\n",
>> + study_cnt_param);
>> + goto error;
>> + }
>> + } else {
>> + /* Default packet compare count when packets count not provided.
>> + * The Api check if the implementation is study and sets it to default
>> + * else returns -EINVAl. */
>> + study_ret = mfex_set_study_pkt_cnt(MFEX_MAX_PKT_COUNT, mfex_name);
>> }
>>
>> ovs_mutex_lock(&dp_netdev_mutex);
>> @@ -1114,7 +1167,6 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> size_t n;
>>
>> sorted_poll_thread_list(dp, &pmd_list, &n);
>> - miniflow_extract_func default_func = dp_mfex_impl_get_default();
>>
>> for (size_t i = 0; i < n; i++) {
>> struct dp_netdev_pmd_thread *pmd = pmd_list[i];
>> @@ -1122,8 +1174,38 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> continue;
>> }
>>
>> + if (pmd_core_param_set) {
>> + if ((pmd_thread_specified) &&
>> + (pmd->core_id != pmd_thread_to_change)) {
>> + continue;
>> + }
>> + }
>> +
>> + /* Set default only when available pmd_core_id was given or
>> + * set for all the pmd core_id if no particular core_id was
>> + * provided else dont set MFEX default.
>> + */
>
> This is still wrong we should only set the default if NO pmd core was given.
> See discussion with Harry.
>
>> + err = dp_mfex_impl_set_default_by_name(mfex_name);
>> + if (err) {
>> + char *error_desc = NULL;
>> + if (err == -EINVAL) {
>> + error_desc = "Unknown miniflow extract implementation:";
>> + } else if (err == -ENOENT) {
>> + error_desc = "Miniflow extract implementation doesn't"
>> + " exist:";
>> + } else if (err == -ENODEV) {
>> + error_desc = "Miniflow extract implementation not"
>> + " available:";
>> + } else {
>> + error_desc = "Miniflow extract implementation Error:";
>> + }
>> + ds_put_format(&reply, "%s %s.\n", error_desc, mfex_name);
>> + }
>> + miniflow_extract_func default_func = dp_mfex_impl_get_default();
>> +
>> /* Initialize MFEX function pointer to the newly configured
>> * default. */
>> + pmd_thread_update_ok = true;
>> atomic_uintptr_t *pmd_func = (void *) &pmd->miniflow_extract_opt;
>> atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
>> };
>> @@ -1131,14 +1213,37 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>
>> ovs_mutex_unlock(&dp_netdev_mutex);
>>
>> + /* If PMD thread was specified, but it wasn't found, return error. */
>> + if (pmd_thread_specified && !pmd_thread_update_ok) {
>> + ds_put_format(&reply,
>> + "Error: Miniflow parser not changed, PMD thread %d"
>> + " not in use, pass a valid pmd thread ID.\n",
>> + pmd_thread_to_change);
>> + goto error;
>> + }
>> +
>> /* Reply with success to command. */
>> - struct ds reply = DS_EMPTY_INITIALIZER;
>> ds_put_format(&reply, "Miniflow Extract implementation set to %s",
>> mfex_name);
>> - const char *reply_str = ds_cstr(&reply);
>> + if (pmd_thread_specified) {
>> + ds_put_format(&reply, ", on pmd thread %d", pmd_thread_to_change);
>> + }
>> + if (study_ret == 0) {
>> + ds_put_format(&reply, ", studying %d packets", pkt_cmp_count);
>> + }
>> +
>> + ds_put_format(&reply, ".\n");
>> +
>> + reply_str = ds_cstr(&reply);
>> VLOG_INFO("%s", reply_str);
>> unixctl_command_reply(conn, reply_str);
>> ds_destroy(&reply);
>> + return;
>
> Add extra newline
>
>> +error:
>> + reply_str = ds_cstr(&reply);
>> + VLOG_ERR("%s", reply_str);
>> + unixctl_command_reply_error(conn, reply_str);
>> + ds_destroy(&reply);
>> }
>>
>> static void
>> @@ -1371,8 +1476,9 @@ dpif_netdev_init(void)
>> 0, 0, dpif_netdev_impl_get,
>> NULL);
>> unixctl_command_register("dpif-netdev/miniflow-parser-set",
>> - "miniflow_implementation_name",
>> - 1, 1, dpif_miniflow_extract_impl_set,
>> + "[-pmd core] miniflow_implementation_name"
>> + " [study_pkt_cnt]",
>> + 1, 5, dpif_miniflow_extract_impl_set,
>> NULL);
>> unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
>> 0, 0, dpif_miniflow_extract_impl_get,
>> --
>> 2.25.1
More information about the dev
mailing list