| Message ID | 20200121141250.26989-6-gautamramk@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sched: add Flow Queue PIE packet scheduler | expand |
From: gautamramk@gmail.com Date: Tue, 21 Jan 2020 19:42:44 +0530 > From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in> > > Rearrange the members of the structures such that they appear in > order of their types. Also, change the order of their > initializations to match the order in which they appear in the > structures. This improves the code's readability and consistency. > > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> What matters for structure member ordering is dense packing and grouping commonly-used-together elements for performance. "order of their types" are none of those things.
On Tue, Jan 21, 2020 at 8:05 PM David Miller <davem@davemloft.net> wrote: > > From: gautamramk@gmail.com > Date: Tue, 21 Jan 2020 19:42:44 +0530 > > > From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in> > > > > Rearrange the members of the structures such that they appear in > > order of their types. Also, change the order of their > > initializations to match the order in which they appear in the > > structures. This improves the code's readability and consistency. > > > > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> > > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> > > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> > > What matters for structure member ordering is dense packing and > grouping commonly-used-together elements for performance. > We shall reorder the variables as per their appearance in the structure and re-submit. Could you elaborate a bit on dense packing? > "order of their types" are none of those things.
Gautam Ramakrishnan <gautamramk@gmail.com> writes: > On Tue, Jan 21, 2020 at 8:05 PM David Miller <davem@davemloft.net> wrote: >> >> From: gautamramk@gmail.com >> Date: Tue, 21 Jan 2020 19:42:44 +0530 >> >> > From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in> >> > >> > Rearrange the members of the structures such that they appear in >> > order of their types. Also, change the order of their >> > initializations to match the order in which they appear in the >> > structures. This improves the code's readability and consistency. >> > >> > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> >> > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> >> > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> >> >> What matters for structure member ordering is dense packing and >> grouping commonly-used-together elements for performance. >> > We shall reorder the variables as per their appearance in the > structure and re-submit. Could you elaborate a bit on dense packing? The compiler will align struct member offsets according to their size, adding padding as necessary to achieve this. So this struct: struct s1 { u32 mem32_1; u64 mem64_1; u32 mem32_2; u64 mem64_2; }; will have 4 bytes of padding after both mem32_1 and mem32_2, whereas this struct: struct s2 { u64 mem64_1; u32 mem32_1; u32 mem32_2; u64 mem64_2; }; won't. So sizeof(struct s1) == 32, and sizeof(struct s2) == 24, and we say that s2 is densely packed, whereas s1 has holes in it. The pahole tool is useful to see the layout of compiled structures (pahole -C). It will also point out any holes. -Toke
From: Gautam Ramakrishnan <gautamramk@gmail.com> Date: Tue, 21 Jan 2020 21:14:50 +0530 > On Tue, Jan 21, 2020 at 8:05 PM David Miller <davem@davemloft.net> wrote: >> >> From: gautamramk@gmail.com >> Date: Tue, 21 Jan 2020 19:42:44 +0530 >> >> > From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in> >> > >> > Rearrange the members of the structures such that they appear in >> > order of their types. Also, change the order of their >> > initializations to match the order in which they appear in the >> > structures. This improves the code's readability and consistency. >> > >> > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> >> > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> >> > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> >> >> What matters for structure member ordering is dense packing and >> grouping commonly-used-together elements for performance. >> > We shall reorder the variables as per their appearance in the > structure and re-submit. Could you elaborate a bit on dense packing? It means eliminating unnecessary padding in the structure. F.e. if you have: u32 x; u64 y; Then 32-bits of wasted space will be inserted after 'x' so that 'y' is properly 64-bit aligned. If in doubt use the 'pahole' tool to see how the structure is laid out. It will show you where unnecessary padding exists as well.
On Tue, Jan 21, 2020 at 9:32 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Gautam Ramakrishnan <gautamramk@gmail.com> writes: > > > On Tue, Jan 21, 2020 at 8:05 PM David Miller <davem@davemloft.net> wrote: > >> > >> From: gautamramk@gmail.com > >> Date: Tue, 21 Jan 2020 19:42:44 +0530 > >> > >> > From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in> > >> > > >> > Rearrange the members of the structures such that they appear in > >> > order of their types. Also, change the order of their > >> > initializations to match the order in which they appear in the > >> > structures. This improves the code's readability and consistency. > >> > > >> > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> > >> > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> > >> > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> > >> > >> What matters for structure member ordering is dense packing and > >> grouping commonly-used-together elements for performance. > >> > > We shall reorder the variables as per their appearance in the > > structure and re-submit. Could you elaborate a bit on dense packing? > > The compiler will align struct member offsets according to their size, > adding padding as necessary to achieve this. > So this struct: > > struct s1 { > u32 mem32_1; > u64 mem64_1; > u32 mem32_2; > u64 mem64_2; > }; > > will have 4 bytes of padding after both mem32_1 and mem32_2, whereas > this struct: > > struct s2 { > u64 mem64_1; > u32 mem32_1; > u32 mem32_2; > u64 mem64_2; > }; > > won't. So sizeof(struct s1) == 32, and sizeof(struct s2) == 24, and we > say that s2 is densely packed, whereas s1 has holes in it. > > The pahole tool is useful to see the layout of compiled structures > (pahole -C). It will also point out any holes. > > -Toke > Thanks Toke. Used the pahole tool. There seem to be no problems with the structures in include/net/pie.h, at least not on an x86_64 system. We might have to change things in sch_pie.c and sch_fq_pie.c though.
On Tue, Jan 21, 2020 at 9:39 PM David Miller <davem@davemloft.net> wrote: > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > Date: Tue, 21 Jan 2020 21:14:50 +0530 > > > On Tue, Jan 21, 2020 at 8:05 PM David Miller <davem@davemloft.net> wrote: > >> > >> From: gautamramk@gmail.com > >> Date: Tue, 21 Jan 2020 19:42:44 +0530 > >> > >> > From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in> > >> > > >> > Rearrange the members of the structures such that they appear in > >> > order of their types. Also, change the order of their > >> > initializations to match the order in which they appear in the > >> > structures. This improves the code's readability and consistency. > >> > > >> > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> > >> > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> > >> > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> > >> > >> What matters for structure member ordering is dense packing and > >> grouping commonly-used-together elements for performance. > >> > > We shall reorder the variables as per their appearance in the > > structure and re-submit. Could you elaborate a bit on dense packing? > > It means eliminating unnecessary padding in the structure. F.e. if > you have: > > u32 x; > u64 y; > > Then 32-bits of wasted space will be inserted after 'x' so that > 'y' is properly 64-bit aligned. > > If in doubt use the 'pahole' tool to see how the structure is > laid out. It will show you where unnecessary padding exists as > well. Thanks David. Do you recommend we discard/keep this patch? pahole reports no problems with or without this patch. However, we'll be correcting issues with other structs in sch_pie.c and sch_fq_pie.c.
diff --git a/include/net/pie.h b/include/net/pie.h index f9c6a44bdb0c..54ba6c6a7514 100644 --- a/include/net/pie.h +++ b/include/net/pie.h @@ -28,13 +28,13 @@ struct pie_params { /* variables used */ struct pie_vars { - u64 prob; /* probability but scaled by u64 limit. */ psched_time_t burst_time; psched_time_t qdelay; psched_time_t qdelay_old; - u64 dq_count; /* measured in bytes */ psched_time_t dq_tstamp; /* drain rate */ + u64 prob; /* probability but scaled by u64 limit. */ u64 accu_prob; /* accumulated drop probability */ + u64 dq_count; /* measured in bytes */ u32 avg_dq_rate; /* bytes per pschedtime tick,scaled */ u32 qlen_old; /* in bytes */ u8 accu_prob_overflows; /* overflows of accu_prob */ @@ -56,11 +56,11 @@ struct pie_skb_cb { static inline void pie_params_init(struct pie_params *params) { - params->alpha = 2; - params->beta = 20; + params->target = PSCHED_NS2TICKS(15 * NSEC_PER_MSEC); /* 15 ms */ params->tupdate = usecs_to_jiffies(15 * USEC_PER_MSEC); /* 15 ms */ params->limit = 1000; /* default of 1000 packets */ - params->target = PSCHED_NS2TICKS(15 * NSEC_PER_MSEC); /* 15 ms */ + params->alpha = 2; + params->beta = 20; params->ecn = false; params->bytemode = false; params->dq_rate_estimator = false; @@ -68,12 +68,12 @@ static inline void pie_params_init(struct pie_params *params) static inline void pie_vars_init(struct pie_vars *vars) { - vars->dq_count = DQCOUNT_INVALID; + /* default of 150 ms in pschedtime */ + vars->burst_time = PSCHED_NS2TICKS(150 * NSEC_PER_MSEC); vars->dq_tstamp = DTIME_INVALID; vars->accu_prob = 0; + vars->dq_count = DQCOUNT_INVALID; vars->avg_dq_rate = 0; - /* default of 150 ms in pschedtime */ - vars->burst_time = PSCHED_NS2TICKS(150 * NSEC_PER_MSEC); vars->accu_prob_overflows = 0; }