| Message ID | 1455199701-25767-1-git-send-email-emmanuel.grumbach@intel.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-02-11 at 16:08 +0200, Emmanuel Grumbach wrote: > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > --- > -static bool codel_should_drop(const struct sk_buff *skb, > - struct Qdisc *sch, > - struct codel_vars *vars, > - struct codel_params *params, > - struct codel_stats *stats, > - codel_time_t now) > +static inline bool codel_should_drop(const struct sk_buff *skb, > + struct Qdisc *sch, > + struct codel_vars *vars, > + struct codel_params *params, > + struct codel_stats *stats, > + codel_time_t now) The lack of inline was done on purpose. This include file is kind of special, being included by codel and fq_codel. Hint : we do not want to force the compiler to inline codel_should_drop() (or any other function). See this file as if it was a .c really.
fixing linux-wireless address ... On 02/11/2016 04:30 PM, Eric Dumazet wrote: > On Thu, 2016-02-11 at 16:08 +0200, Emmanuel Grumbach wrote: >> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> >> --- >> -static bool codel_should_drop(const struct sk_buff *skb, >> - struct Qdisc *sch, >> - struct codel_vars *vars, >> - struct codel_params *params, >> - struct codel_stats *stats, >> - codel_time_t now) >> +static inline bool codel_should_drop(const struct sk_buff *skb, >> + struct Qdisc *sch, >> + struct codel_vars *vars, >> + struct codel_params *params, >> + struct codel_stats *stats, >> + codel_time_t now) > > The lack of inline was done on purpose. > > This include file is kind of special, being included by codel and > fq_codel. > > Hint : we do not want to force the compiler to inline > codel_should_drop() (or any other function). > > > See this file as if it was a .c really. > > Yeah :) codel_should_drop seemed very long indeed... I wanted to use the codel_get_time and associated utils (_before, _after) in iwlwifi. They're better than jiffies... So maybe I can just copy that code to iwlwifi.
On Thu, 2016-02-11 at 15:05 +0000, Grumbach, Emmanuel wrote: > Yeah :) codel_should_drop seemed very long indeed... I wanted to use the > codel_get_time and associated utils (_before, _after) in iwlwifi. > They're better than jiffies... So maybe I can just copy that code to > iwlwifi. You certainly can submit a patch adding the inline, but not on all functions present in this file ;) Thanks !
On 02/11/2016 05:12 PM, Eric Dumazet wrote: > On Thu, 2016-02-11 at 15:05 +0000, Grumbach, Emmanuel wrote: > > >> Yeah :) codel_should_drop seemed very long indeed... I wanted to use the >> codel_get_time and associated utils (_before, _after) in iwlwifi. >> They're better than jiffies... So maybe I can just copy that code to >> iwlwifi. > > You certainly can submit a patch adding the inline, but not on all > functions present in this file ;) > > Thanks ! > Actually... All I need *has* the inline, but if I include codel.h, codel_dequeue is defined but not used and you definitely don't want to inline that one. So I guess the only other option I have is to split that header file which I don't think is really worth it. So, unless you object it, I'll just copy the code.
On Thu, Feb 11, 2016 at 7:05 AM, Grumbach, Emmanuel <emmanuel.grumbach@intel.com> wrote: > fixing linux-wireless address ... > > On 02/11/2016 04:30 PM, Eric Dumazet wrote: >> On Thu, 2016-02-11 at 16:08 +0200, Emmanuel Grumbach wrote: >>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> >>> --- >>> -static bool codel_should_drop(const struct sk_buff *skb, >>> - struct Qdisc *sch, >>> - struct codel_vars *vars, >>> - struct codel_params *params, >>> - struct codel_stats *stats, >>> - codel_time_t now) >>> +static inline bool codel_should_drop(const struct sk_buff *skb, >>> + struct Qdisc *sch, >>> + struct codel_vars *vars, >>> + struct codel_params *params, >>> + struct codel_stats *stats, >>> + codel_time_t now) >> >> The lack of inline was done on purpose. >> >> This include file is kind of special, being included by codel and >> fq_codel. >> >> Hint : we do not want to force the compiler to inline >> codel_should_drop() (or any other function). >> >> >> See this file as if it was a .c really. >> >> > > Yeah :) codel_should_drop seemed very long indeed... I wanted to use the > codel_get_time and associated utils (_before, _after) in iwlwifi. > They're better than jiffies... So maybe I can just copy that code to > iwlwifi. I need to stress that codel as is is not the right thing for wifi, particularly point to multipoint wifi in highly contended scenarios. It IS a starting point. We have generally felt that the target needs to be offset against the actual service opportunities, and the effects of multicast (with powersave) and other "background" frames, needs to be smoothed out. Lacking hardware that can do that, or adaquate sims, has stalled trying to come up with "the right thing". It looks like you are putting in place more of the pieces to get there in some tree somewhere?
On Thu, Feb 11, 2016 at 7:29 AM, Grumbach, Emmanuel <emmanuel.grumbach@intel.com> wrote: > > > On 02/11/2016 05:12 PM, Eric Dumazet wrote: >> On Thu, 2016-02-11 at 15:05 +0000, Grumbach, Emmanuel wrote: >> >> >>> Yeah :) codel_should_drop seemed very long indeed... I wanted to use the >>> codel_get_time and associated utils (_before, _after) in iwlwifi. >>> They're better than jiffies... So maybe I can just copy that code to >>> iwlwifi. Definately better than jiffies. >> >> You certainly can submit a patch adding the inline, but not on all >> functions present in this file ;) >> >> Thanks ! >> > > Actually... All I need *has* the inline, but if I include codel.h, > codel_dequeue is defined but not used and you definitely don't want to > inline that one. So I guess the only other option I have is to split > that header file which I don't think is really worth it. So, unless you > object it, I'll just copy the code. I think it is best to start with another base implementation of codel for wifi, yes. What I think is the currently best performing codel implementation (on the wire, for ethernet) we have is in: https://github.com/dtaht/bcake/codel5.h which has a few differences from eric's implementation (64 bit timestamps, inlining, not a lot of cpu profiling on it - still aiming for algorithmic correctness here, it is closer to the original paper... We'd used a different means of injecting the callback in it, too) The one currently in the main cake had issues in the last test round but has been updated since. (sch_cake is also on github). In neither case it is the right thing for wifi either. the "starting plan" such as it was was to get to "one aggregate in the hardware, one in the driver, one ready to be formed on the completion interrupt", and pull a smoothed service time from start to completion interrupt to dynamically modify the codel target. (other headaches, like multicast, abound). (It's the per station queue + fq as close to the hardware as possible that matters most, IMHO.) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/codel.h b/include/net/codel.h index 267e702..0775c24 100644 --- a/include/net/codel.h +++ b/include/net/codel.h @@ -92,18 +92,18 @@ struct codel_skb_cb { codel_time_t enqueue_time; }; -static struct codel_skb_cb *get_codel_cb(const struct sk_buff *skb) +static inline struct codel_skb_cb *get_codel_cb(const struct sk_buff *skb) { qdisc_cb_private_validate(skb, sizeof(struct codel_skb_cb)); return (struct codel_skb_cb *)qdisc_skb_cb(skb)->data; } -static codel_time_t codel_get_enqueue_time(const struct sk_buff *skb) +static inline codel_time_t codel_get_enqueue_time(const struct sk_buff *skb) { return get_codel_cb(skb)->enqueue_time; } -static void codel_set_enqueue_time(struct sk_buff *skb) +static inline void codel_set_enqueue_time(struct sk_buff *skb) { get_codel_cb(skb)->enqueue_time = codel_get_time(); } @@ -174,8 +174,8 @@ struct codel_stats { #define CODEL_DISABLED_THRESHOLD INT_MAX -static void codel_params_init(struct codel_params *params, - const struct Qdisc *sch) +static inline void codel_params_init(struct codel_params *params, + const struct Qdisc *sch) { params->interval = MS2TIME(100); params->target = MS2TIME(5); @@ -184,12 +184,12 @@ static void codel_params_init(struct codel_params *params, params->ecn = false; } -static void codel_vars_init(struct codel_vars *vars) +static inline void codel_vars_init(struct codel_vars *vars) { memset(vars, 0, sizeof(*vars)); } -static void codel_stats_init(struct codel_stats *stats) +static inline void codel_stats_init(struct codel_stats *stats) { stats->maxpacket = 0; } @@ -200,7 +200,7 @@ static void codel_stats_init(struct codel_stats *stats) * * Here, invsqrt is a fixed point number (< 1.0), 32bit mantissa, aka Q0.32 */ -static void codel_Newton_step(struct codel_vars *vars) +static inline void codel_Newton_step(struct codel_vars *vars) { u32 invsqrt = ((u32)vars->rec_inv_sqrt) << REC_INV_SQRT_SHIFT; u32 invsqrt2 = ((u64)invsqrt * invsqrt) >> 32; @@ -217,19 +217,19 @@ static void codel_Newton_step(struct codel_vars *vars) * We maintain in rec_inv_sqrt the reciprocal value of sqrt(count) to avoid * both sqrt() and divide operation. */ -static codel_time_t codel_control_law(codel_time_t t, - codel_time_t interval, - u32 rec_inv_sqrt) +static inline codel_time_t codel_control_law(codel_time_t t, + codel_time_t interval, + u32 rec_inv_sqrt) { return t + reciprocal_scale(interval, rec_inv_sqrt << REC_INV_SQRT_SHIFT); } -static bool codel_should_drop(const struct sk_buff *skb, - struct Qdisc *sch, - struct codel_vars *vars, - struct codel_params *params, - struct codel_stats *stats, - codel_time_t now) +static inline bool codel_should_drop(const struct sk_buff *skb, + struct Qdisc *sch, + struct codel_vars *vars, + struct codel_params *params, + struct codel_stats *stats, + codel_time_t now) { bool ok_to_drop; @@ -265,11 +265,11 @@ static bool codel_should_drop(const struct sk_buff *skb, typedef struct sk_buff * (*codel_skb_dequeue_t)(struct codel_vars *vars, struct Qdisc *sch); -static struct sk_buff *codel_dequeue(struct Qdisc *sch, - struct codel_params *params, - struct codel_vars *vars, - struct codel_stats *stats, - codel_skb_dequeue_t dequeue_func) +static inline struct sk_buff *codel_dequeue(struct Qdisc *sch, + struct codel_params *params, + struct codel_vars *vars, + struct codel_stats *stats, + codel_skb_dequeue_t dequeue_func) { struct sk_buff *skb = dequeue_func(vars, sch); codel_time_t now;
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- include/net/codel.h | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)