Hi Ilya,
These patches were already committed into the OVS-DPDK sub-tree that
Darrell is maintaining after they were under review for a long time (4
months and 7 revisions). I will reply to each comment and address the
comments around correctness by way of follow up patches.
thanks,
Kevin.
On 08/28/2017 02:28 PM, Ilya Maximets wrote:
>> Add counters to dp_netdev_rxq which will later be used for storing the
>> processing cycles of an rxq. Processing cycles will be stored in reference
>> to a defined time interval. We will store the cycles of the current in
>> progress
>> interval, a number of completed intervals and the sum of the completed
>> intervals.
>>
>> cycles_count_intermediate was used to count cycles for a pmd. With some small
>> additions we can also use it to count the cycles used for processing an rxq.
>>
>> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
>> ---
>> lib/dpif-netdev.c | 30 +++++++++++++++++++++++++++---
>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index f35c079..8731435 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -182,4 +182,8 @@ struct emc_cache {
>> #define DPCLS_OPTIMIZATION_INTERVAL 1000
>>
>> +/* Number of intervals for which cycles are stored
>> + * and used during rxq to pmd assignment. */
>> +#define PMD_RXQ_INTERVAL_MAX 6
>> +
>> struct dpcls {
>> struct cmap_node node; /* Within dp_netdev_pmd_thread.classifiers
>> */
>> @@ -340,4 +344,13 @@ enum pmd_cycles_counter_type {
>> };
>>
>> +enum rxq_cycles_counter_type {
>> + RXQ_CYCLES_PROC_CURR, /* Cycles spent successfully polling and
>> + processing packets during the current
>> + interval. */
>> + RXQ_CYCLES_PROC_HIST, /* Total cycles of all intervals that are
>> used
>> + during rxq to pmd assignment. */
>> + RXQ_N_CYCLES
>> +};
>
> All patches wide: Multi-line comments should have a '*' on each line.
>
This struct follows the coding style "comments" section example struct.
I'll check the other ones.
>> +
>> #define XPS_TIMEOUT_MS 500LL
>>
>> @@ -351,4 +364,11 @@ struct dp_netdev_rxq {
>> particular core. */
>> struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue.
>> */
>> +
>> + /* Counters of cycles spent successfully polling and processing pkts. */
>> + atomic_ullong cycles[RXQ_N_CYCLES];
>> + /* We store PMD_RXQ_INTERVAL_MAX intervals of data for an rxq and then
>> + sum them to yield the cycles used for an rxq. */
>> + atomic_ullong cycles_intrvl[PMD_RXQ_INTERVAL_MAX];
>> + unsigned intrvl_idx; /* Write index for 'cycles_intrvl'.
>> */
>
> Does it matter to save 2 letters in a variable names? It looks ugly and
> unreadable.
>
I think the current name is ok. Maybe not perfect, but ok.
>> };
>>
>> @@ -677,5 +697,4 @@ static void pmd_load_cached_ports(struct
>> dp_netdev_pmd_thread *pmd)
>> static inline void
>> dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd);
>> -
>
> Is it necessary to remove this line? It was here to split xps related
> functions
> from others.
>
no, it wasn't necessary to remove, but not worthwhile of patch now.
>> static void
>> dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
>> @@ -3092,4 +3111,5 @@ cycles_count_end(struct dp_netdev_pmd_thread *pmd,
>> static inline void
>> cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd,
>> + struct dp_netdev_rxq *rxq,
>> enum pmd_cycles_counter_type type)
>> OVS_NO_THREAD_SAFETY_ANALYSIS
>> @@ -3100,4 +3120,8 @@ cycles_count_intermediate(struct dp_netdev_pmd_thread
>> *pmd,
>>
>> non_atomic_ullong_add(&pmd->cycles.n[type], interval);
>> + if (rxq && (type == PMD_CYCLES_PROCESSING)) {
>> + /* Add to the amount of current processing cycles. */
>> + non_atomic_ullong_add(&rxq->cycles[RXQ_CYCLES_PROC_CURR], interval);
>> + }
>> }
>>
>> @@ -3668,5 +3692,5 @@ dpif_netdev_run(struct dpif *dpif)
>> port->rxqs[i].rx,
>> port->port_no);
>> - cycles_count_intermediate(non_pmd, process_packets ?
>> + cycles_count_intermediate(non_pmd, NULL,
>> process_packets ?
>> PMD_CYCLES_PROCESSING
>> : PMD_CYCLES_IDLE);
>
> It's not yours, but it'll be nice to fix here:
> According to coding style, '?' should be on the next line near to arguments.
> Also, IMHO, the whole construction should have the same level of indention.
>
> Like this:
> cycles_count_intermediate(non_pmd, NULL,
> process_packets
> ? PMD_CYCLES_PROCESSING
> : PMD_CYCLES_IDLE);
> Or this:
> cycles_count_intermediate(
>
> non_pmd, NULL, process_packets ?
> PMD_CYCLES_PROCESSING
> : PMD_CYCLES_IDLE);
>
I don't generally like to mix fixes with new code, even if it's small as
a reviewer has to think about it and it alters the commit message.
There's another coding style issue you caught elsewhere, so I'll fix
them together in a follow up patch.
>> @@ -3855,5 +3879,5 @@ reload:
>> dp_netdev_process_rxq_port(pmd, poll_list[i].rxq->rx,
>> poll_list[i].port_no);
>> - cycles_count_intermediate(pmd,
>> + cycles_count_intermediate(pmd, NULL,
>> process_packets ?
>> PMD_CYCLES_PROCESSING
>> : PMD_CYCLES_IDLE);
>> --
>> 1.8.3.1
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
