[ovs-dev] [PATCH v5 2/2] datapath: Per NUMA node flow stats.

Jarno Rajahalme jrajahalme at nicira.com
Tue Feb 11 00:19:32 UTC 2014

On Feb 10, 2014, at 3:21 PM, Jesse Gross <jesse at nicira.com> wrote:

> On Mon, Feb 10, 2014 at 11:31 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> On Feb 8, 2014, at 7:01 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> On Thu, Feb 6, 2014 at 3:13 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>>>> diff --git a/datapath/flow.h b/datapath/flow.h
>>>> index eafcfd8..f6cce35 100644
>>>> --- a/datapath/flow.h
>>>> +++ b/datapath/flow.h
>>>> @@ -155,14 +155,9 @@ struct flow_stats {
>>>>       unsigned long used;             /* Last used time (in jiffies). */
>>>>       spinlock_t lock;                /* Lock for atomic stats update. */
>>>>       __be16 tcp_flags;               /* Union of seen TCP flags. */
>>>> -};
>>>> -
>>>> -struct sw_flow_stats {
>>>> -       bool is_percpu;
>>>> -       union {
>>>> -               struct flow_stats *stat;
>>>> -               struct flow_stats __percpu *cpu_stats;
>>>> -       };
>>>> +       int last_writer;                /* NUMA-node id of the last writer or
>>>> +                                        * -1. Meaningful for 'stats[0]' only.
>>>> +                                        */
>>> Should we put last_writer directly in struct sw_flow stats? It would
>>> reduce the size a little bit and might even be a little clearer.
>> This patch actually removes the struct sw_flow_stats, and the ‘last_writer’ is part of the struct flow_stats. The alternative to put it to struct sw_flow would use more space (flow_stats are allocated cache line aligned, so there is some slack), and make the struct sw_flow a bit more volatile than it needs to be. To be clear, the definitions now look like this:
> Sorry, that was a typo. I meant to say put it in struct sw_flow as you guessed.
> It sounds like you are planning on removing the cacheline alignment
> for flow_stats, so moving it would help save a little space. As far as
> writing to sw_flow frequently, I guess this would primarily happen if
> we have an allocation failure right (since if it wasn't in struct
> flow_stats then we probably wouldn't access it on every stats update
> anyways)? Is it possible to minimize the effects in this corner case?

Your earlier comment was about the cacheline alignment of the struct sw_flow itself. I still think the stats should be cacheline aligned, as they will be written to constantly, and many times by multiple cores (on the same NUMA node). The ‘last_writer’ is specific to the individual stats entry, so moving them to struct flow would require an array of them alongside the stats pointer array.

I’ll send the v6 for you to review, so we do not discuss code that has not been posted yet.


More information about the dev mailing list