Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount_t

2018-06-15 Thread Kevin Darbyshire-Bryant 


> On 15 Jun 2018, at 21:57, David Woodhouse  wrote:
> 
> 
> 
> On Fri, 2018-06-15 at 20:49 +, Kevin Darbyshire-Bryant wrote:
>> 
>>> That does end up being quite hairy. I don't think it's worth doing.
>>> 
>>> This should probably suffice to fix it...
>>> 
>>> Kevin this is going to conflict with the ifx_atm_alloc_skb() hack in
>>> the tree you're working on, but that needs to be killed with fire
>>> anyway. It's utterly pointless as discussed.
>> 
>> I had already done so as part of the last pastebin debug info round :-)
>> 
>> As regards your patch… MAGIC!  Works an absolute treat.  Will get
>> that submitted along with the ‘nuke ifx_atm_alloc_skb’ patch to
>> OpenWrt tomorrow.  For now, maybe my brain will let me sleep :-)
>> 
>> Thank you soo much for your help & patience.
>> 
>> Tested-by: Kevin Darbyshire-Bryant 
> 
> Thanks. In the morning please could I trouble you to test the other
> variants that you can manage — PPPoA with llc-encap, as well as br2684
> and PPPoE over that?

I can confirm that PPPoA with both vc & llc encapsulations work.  BR2684 with 
PPPoE and both vc & llc encapsulations also work.  No nasty messages noted in 
dmesg.  I’m actually gobsmacked at how tolerant TalkTalk/BT are of what I’ve 
thrown at them, they clearly just look for PPP frames :-)

Kevin



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount_t

2018-06-15 Thread Kevin Darbyshire-Bryant 


> On 15 Jun 2018, at 21:00, David Woodhouse  wrote:
> 
> On Fri, 2018-06-15 at 14:44 +0100, David Woodhouse wrote:
>> 
>>> Or simply use a new field in ATM_SKB(skb) to remember a stable
>>> truesize used in both sides (add/sub)
>> 
>> Right, that was my second suggestion ("copy the accounted value...").
>> 
>> It's a bit of a hack, and I think that actually *using* sock_wfree()
>> instead of what's currently in atm_pop_raw() would be the better
>> solution. Does anyone remember why we didn't do that in the first
>> place?
> 
> That does end up being quite hairy. I don't think it's worth doing.
> 
> This should probably suffice to fix it...
> 
> Kevin this is going to conflict with the ifx_atm_alloc_skb() hack in
> the tree you're working on, but that needs to be killed with fire
> anyway. It's utterly pointless as discussed.

I had already done so as part of the last pastebin debug info round :-)

As regards your patch… MAGIC!  Works an absolute treat.  Will get that 
submitted along with the ‘nuke ifx_atm_alloc_skb’ patch to OpenWrt tomorrow.  
For now, maybe my brain will let me sleep :-)

Thank you soo much for your help & patience.

Tested-by: Kevin Darbyshire-Bryant 

> 
> 
> From 3368eaeb0a2f09138894dde0f26f879e5228005a Mon Sep 17 00:00:00 2001
> From: David Woodhouse 
> Date: Fri, 15 Jun 2018 20:49:20 +0100
> Subject: [PATCH] atm: Preserve value of skb->truesize when accounting to vcc
> 
> There's a hack in pskb_expand_head() to avoid adjusting skb->truesize
> for certain skbs. Ideally it would cover ATM too. It doesn't. Just
> stashing the accounted value and using it in atm_raw_pop() is probably
> the easiest way to cope.
> 
> Signed-off-by: David Woodhouse 
> ---
> include/linux/atmdev.h | 15 +++
> net/atm/br2684.c   |  3 +--
> net/atm/clip.c |  3 +--
> net/atm/common.c   |  3 +--
> net/atm/lec.c  |  3 +--
> net/atm/mpc.c  |  3 +--
> net/atm/pppoatm.c  |  3 +--
> net/atm/raw.c  |  4 ++--
> 8 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
> index 0c27515d2cf6..8124815eb121 100644
> --- a/include/linux/atmdev.h
> +++ b/include/linux/atmdev.h
> @@ -214,6 +214,7 @@ struct atmphy_ops {
> struct atm_skb_data {
>   struct atm_vcc  *vcc;   /* ATM VCC */
>   unsigned long   atm_options;/* ATM layer options */
> + unsigned intacct_truesize;  /* truesize accounted to vcc */
> };
> 
> #define VCC_HTABLE_SIZE 32
> @@ -241,6 +242,20 @@ void vcc_insert_socket(struct sock *sk);
> 
> void atm_dev_release_vccs(struct atm_dev *dev);
> 
> +static inline void atm_account_tx(struct atm_vcc *vcc, struct sk_buff *skb)
> +{
> + /*
> +  * Because ATM skbs may not belong to a sock (and we don't
> +  * necessarily want to), skb->truesize may be adjusted,
> +  * escaping the hack in pskb_expand_head() which avoids
> +  * doing so for some cases. So stash the value of truesize
> +  * at the time we accounted it, and atm_pop_raw() can use
> +  * that value later, in case it changes.
> +  */
> + refcount_add(skb->truesize, _atm(vcc)->sk_wmem_alloc);
> + ATM_SKB(skb)->acct_truesize = skb->truesize;
> + ATM_SKB(skb)->atm_options = vcc->atm_options;
> +}
> 
> static inline void atm_force_charge(struct atm_vcc *vcc,int truesize)
> {
> diff --git a/net/atm/br2684.c b/net/atm/br2684.c
> index 4e96f902..bc21f8e8daf2 100644
> --- a/net/atm/br2684.c
> +++ b/net/atm/br2684.c
> @@ -252,8 +252,7 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct 
> net_device *dev,
> 
>   ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc;
>   pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev);
> - refcount_add(skb->truesize, _atm(atmvcc)->sk_wmem_alloc);
> - ATM_SKB(skb)->atm_options = atmvcc->atm_options;
> + atm_account_tx(atmvcc, skb);
>   dev->stats.tx_packets++;
>   dev->stats.tx_bytes += skb->len;
> 
> diff --git a/net/atm/clip.c b/net/atm/clip.c
> index 65f706e4344c..60920a42f640 100644
> --- a/net/atm/clip.c
> +++ b/net/atm/clip.c
> @@ -381,8 +381,7 @@ static netdev_tx_t clip_start_xmit(struct sk_buff *skb,
>   memcpy(here, llc_oui, sizeof(llc_oui));
>   ((__be16 *) here)[3] = skb->protocol;
>   }
> - refcount_add(skb->truesize, _atm(vcc)->sk_wmem_alloc);
> - ATM_SKB(skb)->atm_options = vcc->atm_options;
> + atm_account_tx(vcc, skb);
>   entry->vccs->last_use = jiffies;
>   pr_deb

Re: [Cake] [PATCH net-next v15 4/7] sch_cake: Add NAT awareness to packet classifier

2018-05-23 Thread Kevin Darbyshire-Bryant 


> On 23 May 2018, at 23:40, Toke Høiland-Jørgensen  wrote:
> 

> 
> Hmm, and we still have an issue with ingress filtering (where cake is
> running on an ifb interface). That runs pre-NAT in the conntrack case,
> and we can't do the RX trick. Here we do the lookup manually in
> conntrack (and this part is actually what brings in most of the
> dependencies). Any neat tricks up your sleeve for this case? :)

I wonder here if our terminology with ‘ingress’ is causing confusion.  For 
avoidance of doubt:

Typical use case of cake on LAN/WAN router requires two instances.  One 
instance (the egress) is on the WAN interface itself.  It is post conntrack and 
hence uses skb->nfct to work out the real pre-nat source address of the LAN 
hosts.

Since we cannot apply this qdisc to the ingress of our WAN interface we use an 
IFB to mirror the ingress packets, and then use a cake instance on the ifb 
interface on its egress path to in essence control the ingress traffic.

Cake has two modes, the normal ‘egress’ mode which is designed to be used when 
controlling egress traffic output, and shapes post any dropped packets.  
‘ingress’ mode is designed to be used on the egress of our ingress IFB, where 
the shaper counts all packets used (well they got here!) even if we decide to 
drop them a bit later.

The ifb positioned cake has the additional fun factor that the conntrack field 
hasn’t yet been filled in, so the qdisc has to go looking in the conntrack 
tables itself to see if any NATting has taken place and balance LAN host 
fairness based on that.

As far as I understand it, the flow dissector doesn’t obviously help with 
working out the pre-NAT addressing as the flow has already been mangled in the 
egress case, and is awaiting mangling on the ingress case.


Kevin


signature.asc
Description: Message signed with OpenPGP

Re: [Cake] [PATCH net-next v12 3/7] sch_cake: Add optional ACK filter

2018-05-18 Thread Kevin Darbyshire-Bryant 


> On 18 May 2018, at 05:27, Cong Wang  wrote:
> 
> On Thu, May 17, 2018 at 4:23 AM, Toke Høiland-Jørgensen  wrote:
>> Eric Dumazet  writes:
>> 
>>> On 05/16/2018 01:29 PM, Toke Høiland-Jørgensen wrote:
 The ACK filter is an optional feature of CAKE which is designed to improve
 performance on links with very asymmetrical rate limits. On such links
 (which are unfortunately quite prevalent, especially for DSL and cable
 subscribers), the downstream throughput can be limited by the number of
 ACKs capable of being transmitted in the *upstream* direction.
 
>>> 
>>> ...
>>> 
 
 Signed-off-by: Toke Høiland-Jørgensen 
 ---
 net/sched/sch_cake.c |  260 
 ++
 1 file changed, 258 insertions(+), 2 deletions(-)
 
 
>>> 
>>> I have decided to implement ACK compression in TCP stack itself.
>> 
>> Awesome! Will look forward to seeing that!
> 
> +1
> 
> It is really odd to put into a TC qdisc, TCP stack is a much better
> place.

Speaking as a user of cake’s ack filtering, although it may be an odd place, it 
is incredibly useful in my linux based home router middle box that usefully 
extracts extra usable bandwidth from my asymmetric link.  And whilst ack 
compression/reduction/filtering call it what you will, will come to the linux 
TCP stack, as yet other OS stacks are less enlightened and benefit from the 
router’s tweaking/meddling/interference.




signature.asc
Description: Message signed with OpenPGP

Re: [PATCH iproute2-next] json_print: fix print_uint with helper type extensions

2018-03-30 Thread Kevin Darbyshire-Bryant 


> On 30 Mar 2018, at 15:57, David Ahern  wrote:
> 
> 
> My comment was to send separate patches - bug fix only for iproute2
> master and then a separate one for enhancements going to -next. If the
> enhancements overlap the bug fix then it needs to wait for the merge.
> This is really no different than what is often needed for net and net-next.

Ahh, okay.  Thanks David.  I understand now.  I don’t contribute to 
net/net-next so it’s a learning curve there as well :-)

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

Re: [PATCH iproute2] json_print: fix print_uint hidden type promotion

2018-03-29 Thread Kevin Darbyshire-Bryant 


> On 29 Mar 2018, at 22:02, Stephen Hemminger <step...@networkplumber.org> 
> wrote:
> 
> On Thu, 29 Mar 2018 20:22:20 +0100
> Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk> wrote:
> 
>> print_int used 'int' type internally, whereas print_uint used 'uint64_t'
>> 
>> These helper functions eventually call vfprintf(fp, fmt, args) which is
>> a variable argument list function and is dependent upon 'fmt' containing
>> correct information about the length of the passed arguments.
>> 
>> Unfortunately print_int v print_uint offered no clue to the programmer
>> that internally passed ints to print_uint were being promoted to 64bits,
>> thus the format passed in 'fmt' string vs the actual passed integer
>> could be different lengths.  This is even more interesting on big endian
>> architectures where 'vfprintf' would be looking in the middle of an
>> int64 type.
>> 
>> print_u/int now stick with native int size.
>> 
>> Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
>> ---
>> include/json_print.h | 2 +-
>> lib/json_print.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/json_print.h b/include/json_print.h
>> index 2ca7830a..45bc653d 100644
>> --- a/include/json_print.h
>> +++ b/include/json_print.h
>> @@ -56,10 +56,10 @@ void close_json_array(enum output_type type, const char 
>> *delim);
>>  print_color_##type_name(t, COLOR_NONE, key, fmt, value);
>> \
>>  }
>> _PRINT_FUNC(int, int);
>> +_PRINT_FUNC(uint, unsigned int);
>> _PRINT_FUNC(bool, bool);
>> _PRINT_FUNC(null, const char*);
>> _PRINT_FUNC(string, const char*);
>> -_PRINT_FUNC(uint, uint64_t);
>> _PRINT_FUNC(hu, unsigned short);
>> _PRINT_FUNC(hex, unsigned int);
>> _PRINT_FUNC(0xhex, unsigned int);
>> diff --git a/lib/json_print.c b/lib/json_print.c
>> index 6518ba98..8d54d1d4 100644
>> --- a/lib/json_print.c
>> +++ b/lib/json_print.c
>> @@ -117,8 +117,8 @@ void close_json_array(enum output_type type, const char 
>> *str)
>>  }   \
>>  }
>> _PRINT_FUNC(int, int);
>> +_PRINT_FUNC(uint, unsigned int);
>> _PRINT_FUNC(hu, unsigned short);
>> -_PRINT_FUNC(uint, uint64_t);
>> _PRINT_FUNC(lluint, unsigned long long int);
>> _PRINT_FUNC(float, double);
>> #undef _PRINT_FUNC
> 
> 
> I am concerned that this will break output of 64 bit statistics on 32 bit 
> hosts.

I honestly don’t know what to do.  Without the patch I see breakage on <33 bit 
stats with 32 bit big endian hosts ‘cos the printf formatting doesn’t know the 
type passed internally by the function is 64bits long. e.g.

tc qdisc
qdisc noqueue 0: dev lo root refcnt 4486716 
qdisc fq_codel 0: dev eth1 root refcnt 4486716 limit 4498840p flows 4536204 
quantum 4539856 target 5.0ms interval 100.0ms memory_limit 4Mb ecn 
qdisc noqueue 0: dev br-lan root refcnt 4486716 
qdisc noqueue 0: dev eth1.2 root refcnt 4486716 
qdisc noqueue 0: dev br-wifi_guest root refcnt 4486716 
qdisc noqueue 0: dev eth1.15 root refcnt 4486716 
qdisc noqueue 0: dev wlan1 root refcnt 4486716 
qdisc noqueue 0: dev wlan0 root refcnt 4486716 
qdisc noqueue 0: dev wlan1-1 root refcnt 4486716 
qdisc noqueue 0: dev wlan0-1 root refcnt 4486716

I guess _PRINT_FUNC(int, int) could be _PRINT_FUNC(int, int64_t) and then at 
least we’d be consistent in doing hidden promotions and see breakage for both 
signed & unsigned types on certain architectures.

But I think I’ve hit my (lack of) skill limit and don’t really know how to take 
this further forward, or wish to break more protocols :-)

Re: [PATCH iproute2-next] json_print: fix print_uint with helper type extensions

2018-03-29 Thread Kevin Darbyshire-Bryant 


> On 29 Mar 2018, at 22:03, Stephen Hemminger <step...@networkplumber.org> 
> wrote:
> 
> On Thu, 29 Mar 2018 20:32:10 +0100
> Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk> wrote:
> 
>> Introduce print helper functions for int, uint, explicit int32, uint32,
>> int64 & uint64.
>> 
>> print_int used 'int' type internally, whereas print_uint used 'uint64_t'
>> 
>> These helper functions eventually call vfprintf(fp, fmt, args) which is
>> a variable argument list function and is dependent upon 'fmt' containing
>> correct information about the length of the passed arguments.
>> 
>> Unfortunately print_int v print_uint offered no clue to the programmer
>> that internally passed ints to print_uint were being promoted to 64bits,
>> thus the format passed in 'fmt' string vs the actual passed integer
>> could be different lengths.  This is even more interesting on big endian
>> architectures where 'vfprintf' would be looking in the middle of an
>> int64 type.
>> 
>> print_u/int now stick with native int size.  print_u/int32 & print
>> u/int64 functions offer explicit integer sizes.
>> 
>> To portably use these formats you should use the relevant PRIdN or PRIuN
>> formats as defined in inttypes.h
>> 
>> e.g.
>> 
>> print_uint64(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info)
>> 
>> Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
>> ---
>> include/json_print.h | 6 +-
>> lib/json_print.c | 6 +-
>> 2 files changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/json_print.h b/include/json_print.h
>> index 2ca7830a..fb62b142 100644
>> --- a/include/json_print.h
>> +++ b/include/json_print.h
>> @@ -56,10 +56,14 @@ void close_json_array(enum output_type type, const char 
>> *delim);
>>  print_color_##type_name(t, COLOR_NONE, key, fmt, value);
>> \
>>  }
>> _PRINT_FUNC(int, int);
>> +_PRINT_FUNC(uint, unsigned int);
>> _PRINT_FUNC(bool, bool);
>> _PRINT_FUNC(null, const char*);
>> _PRINT_FUNC(string, const char*);
>> -_PRINT_FUNC(uint, uint64_t);
>> +_PRINT_FUNC(int32, int32_t);
>> +_PRINT_FUNC(uint32, uint32_t);
>> +_PRINT_FUNC(int64, int64_t);
>> +_PRINT_FUNC(uint64, uint64_t);
>> _PRINT_FUNC(hu, unsigned short);
>> _PRINT_FUNC(hex, unsigned int);
>> _PRINT_FUNC(0xhex, unsigned int);
>> diff --git a/lib/json_print.c b/lib/json_print.c
>> index bda72933..1194a6ec 100644
>> --- a/lib/json_print.c
>> +++ b/lib/json_print.c
>> @@ -116,8 +116,12 @@ void close_json_array(enum output_type type, const char 
>> *str)
>>  }   \
>>  }
>> _PRINT_FUNC(int, int);
>> +_PRINT_FUNC(uint, unsigned int);
>> _PRINT_FUNC(hu, unsigned short);
>> -_PRINT_FUNC(uint, uint64_t);
>> +_PRINT_FUNC(int32, int32_t);
>> +_PRINT_FUNC(uint32, uint32_t);
>> +_PRINT_FUNC(int64, int64_t);
>> +_PRINT_FUNC(uint64, uint64_t);
>> _PRINT_FUNC(lluint, unsigned long long int);
>> _PRINT_FUNC(float, double);
>> #undef _PRINT_FUNC
> 
> You sent patches to both trees. That is not the correct protocol.
> Choose one, get it reviewed.  iproute2-next will get merged from master (in 
> fact
> dave should be doing it regularly).

I got this from Dave "Kevin: I guess you need to split the patch. Extract the 
bug fix piece
and send for iproute2; enhancements go to iproute2-next.”

So I thought I was doing the right thing.

But to be blunt, I’m giving up now.


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

Re: [PATCH] json_print: fix print_uint with helper type extensions

2018-03-29 Thread Kevin Darbyshire-Bryant 


> On 26 Mar 2018, at 16:03, David Ahern <dsah...@gmail.com> wrote:
> 
> On 3/18/18 2:40 AM, Kevin Darbyshire-Bryant wrote:
>> 
>> 
>>> On 16 Mar 2018, at 20:34, Stephen Hemminger <step...@networkplumber.org> 
>>> wrote:
>>>> 
>>>> print_uint64(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info)
>>>> 
>>>> Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
>>> 
>>> I am fine with this. But since there is no code using it yet, it should
>>> go net-next branch.
>>> 
>>> Reviewed-by: Stephen Hemminger <step...@networkplumber.org>
>> 
>> Existing code is tripping up over the hidden uint - > uint64_t promotion in 
>> print_uint in iproute2 v4.15, that’s how I fell over the issue.  Should I 
>> split the patch?  One fixing the uint->uint64_t and the other offering the 
>> explicit type length options.
>> 
>> Obviously I now realise that the email header should have iproute2 in it.  
>> Learning, slowly :-)
>> 
> 
> Kevin: I guess you need to split the patch. Extract the bug fix piece
> and send for iproute2; enhancements go to iproute2-next.

Done that - hopefully done it right or at least better.


012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A



signature.asc
Description: Message signed with OpenPGP

[PATCH iproute2-next] json_print: fix print_uint with helper type extensions

2018-03-29 Thread Kevin Darbyshire-Bryant 
Introduce print helper functions for int, uint, explicit int32, uint32,
int64 & uint64.

print_int used 'int' type internally, whereas print_uint used 'uint64_t'

These helper functions eventually call vfprintf(fp, fmt, args) which is
a variable argument list function and is dependent upon 'fmt' containing
correct information about the length of the passed arguments.

Unfortunately print_int v print_uint offered no clue to the programmer
that internally passed ints to print_uint were being promoted to 64bits,
thus the format passed in 'fmt' string vs the actual passed integer
could be different lengths.  This is even more interesting on big endian
architectures where 'vfprintf' would be looking in the middle of an
int64 type.

print_u/int now stick with native int size.  print_u/int32 & print
u/int64 functions offer explicit integer sizes.

To portably use these formats you should use the relevant PRIdN or PRIuN
formats as defined in inttypes.h

e.g.

print_uint64(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info)

Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
---
 include/json_print.h | 6 +-
 lib/json_print.c | 6 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/json_print.h b/include/json_print.h
index 2ca7830a..fb62b142 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -56,10 +56,14 @@ void close_json_array(enum output_type type, const char 
*delim);
print_color_##type_name(t, COLOR_NONE, key, fmt, value);
\
}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(bool, bool);
 _PRINT_FUNC(null, const char*);
 _PRINT_FUNC(string, const char*);
-_PRINT_FUNC(uint, uint64_t);
+_PRINT_FUNC(int32, int32_t);
+_PRINT_FUNC(uint32, uint32_t);
+_PRINT_FUNC(int64, int64_t);
+_PRINT_FUNC(uint64, uint64_t);
 _PRINT_FUNC(hu, unsigned short);
 _PRINT_FUNC(hex, unsigned int);
 _PRINT_FUNC(0xhex, unsigned int);
diff --git a/lib/json_print.c b/lib/json_print.c
index bda72933..1194a6ec 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -116,8 +116,12 @@ void close_json_array(enum output_type type, const char 
*str)
}   \
}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(hu, unsigned short);
-_PRINT_FUNC(uint, uint64_t);
+_PRINT_FUNC(int32, int32_t);
+_PRINT_FUNC(uint32, uint32_t);
+_PRINT_FUNC(int64, int64_t);
+_PRINT_FUNC(uint64, uint64_t);
 _PRINT_FUNC(lluint, unsigned long long int);
 _PRINT_FUNC(float, double);
 #undef _PRINT_FUNC
-- 
2.14.3 (Apple Git-98)

[PATCH iproute2] json_print: fix print_uint hidden type promotion

2018-03-29 Thread Kevin Darbyshire-Bryant 
print_int used 'int' type internally, whereas print_uint used 'uint64_t'

These helper functions eventually call vfprintf(fp, fmt, args) which is
a variable argument list function and is dependent upon 'fmt' containing
correct information about the length of the passed arguments.

Unfortunately print_int v print_uint offered no clue to the programmer
that internally passed ints to print_uint were being promoted to 64bits,
thus the format passed in 'fmt' string vs the actual passed integer
could be different lengths.  This is even more interesting on big endian
architectures where 'vfprintf' would be looking in the middle of an
int64 type.

print_u/int now stick with native int size.

Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
---
 include/json_print.h | 2 +-
 lib/json_print.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/json_print.h b/include/json_print.h
index 2ca7830a..45bc653d 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -56,10 +56,10 @@ void close_json_array(enum output_type type, const char 
*delim);
print_color_##type_name(t, COLOR_NONE, key, fmt, value);
\
}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(bool, bool);
 _PRINT_FUNC(null, const char*);
 _PRINT_FUNC(string, const char*);
-_PRINT_FUNC(uint, uint64_t);
 _PRINT_FUNC(hu, unsigned short);
 _PRINT_FUNC(hex, unsigned int);
 _PRINT_FUNC(0xhex, unsigned int);
diff --git a/lib/json_print.c b/lib/json_print.c
index 6518ba98..8d54d1d4 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -117,8 +117,8 @@ void close_json_array(enum output_type type, const char 
*str)
}   \
}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(hu, unsigned short);
-_PRINT_FUNC(uint, uint64_t);
 _PRINT_FUNC(lluint, unsigned long long int);
 _PRINT_FUNC(float, double);
 #undef _PRINT_FUNC
-- 
2.14.3 (Apple Git-98)

Re: [PATCH] json_print: fix print_uint with helper type extensions

2018-03-18 Thread Kevin Darbyshire-Bryant 


> On 16 Mar 2018, at 20:34, Stephen Hemminger <step...@networkplumber.org> 
> wrote:
>> 
>> print_uint64(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info)
>> 
>> Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
> 
> I am fine with this. But since there is no code using it yet, it should
> go net-next branch.
> 
> Reviewed-by: Stephen Hemminger <step...@networkplumber.org>

Existing code is tripping up over the hidden uint - > uint64_t promotion in 
print_uint in iproute2 v4.15, that’s how I fell over the issue.  Should I split 
the patch?  One fixing the uint->uint64_t and the other offering the explicit 
type length options.

Obviously I now realise that the email header should have iproute2 in it.  
Learning, slowly :-)

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A



signature.asc
Description: Message signed with OpenPGP

[PATCH] json_print: fix print_uint with helper type extensions

2018-03-15 Thread Kevin Darbyshire-Bryant 
Introduce print helper functions for int, uint, explicit int32, uint32,
int64 & uint64.

print_int used 'int' type internally, whereas print_uint used 'uint64_t'

These helper functions eventually call vfprintf(fp, fmt, args) which is
a variable argument list function and is dependent upon 'fmt' containing
correct information about the length of the passed arguments.

Unfortunately print_int v print_uint offered no clue to the programmer
that internally passed ints to print_uint were being promoted to 64bits,
thus the format passed in 'fmt' string vs the actual passed integer
could be different lengths.  This is even more interesting on big endian
architectures where 'vfprintf' would be looking in the middle of an
int64 type and hence produced wildly incorrect values in tc qdisc
output.

print_u/int now stick with native int size.  print_u/int32 & print
u/int64 functions offer explicit integer sizes.

To portably use these formats you should use the relevant PRIdN or PRIuN
formats as defined in inttypes.h

e.g.

print_uint64(PRINT_ANY, "refcnt", "refcnt %" PRIu64 " ", t->tcm_info)

Signed-off-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
---
 include/json_print.h | 6 +-
 lib/json_print.c | 6 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/json_print.h b/include/json_print.h
index 2ca7830a..fb62b142 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -56,10 +56,14 @@ void close_json_array(enum output_type type, const char 
*delim);
print_color_##type_name(t, COLOR_NONE, key, fmt, value);
\
}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(bool, bool);
 _PRINT_FUNC(null, const char*);
 _PRINT_FUNC(string, const char*);
-_PRINT_FUNC(uint, uint64_t);
+_PRINT_FUNC(int32, int32_t);
+_PRINT_FUNC(uint32, uint32_t);
+_PRINT_FUNC(int64, int64_t);
+_PRINT_FUNC(uint64, uint64_t);
 _PRINT_FUNC(hu, unsigned short);
 _PRINT_FUNC(hex, unsigned int);
 _PRINT_FUNC(0xhex, unsigned int);
diff --git a/lib/json_print.c b/lib/json_print.c
index 6518ba98..12ee26df 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -117,8 +117,12 @@ void close_json_array(enum output_type type, const char 
*str)
}   \
}
 _PRINT_FUNC(int, int);
+_PRINT_FUNC(uint, unsigned int);
 _PRINT_FUNC(hu, unsigned short);
-_PRINT_FUNC(uint, uint64_t);
+_PRINT_FUNC(int32, int32_t);
+_PRINT_FUNC(uint32, uint32_t);
+_PRINT_FUNC(int64, int64_t);
+_PRINT_FUNC(uint64, uint64_t);
 _PRINT_FUNC(lluint, unsigned long long int);
 _PRINT_FUNC(float, double);
 #undef _PRINT_FUNC
-- 
2.14.3 (Apple Git-98)

Re: OpenWRT wrong adjustment of fq_codel defaults (Was: [Codel] fq_codel_drop vs a udp flood)

2016-05-07 Thread Kevin Darbyshire-Bryant 


On 06/05/16 10:42, Jesper Dangaard Brouer wrote:
> Hi Felix,
>
> This is an important fix for OpenWRT, please read!
>
> OpenWRT changed the default fq_codel sch->limit from 10240 to 1024,
> without also adjusting q->flows_cnt.  Eric explains below that you must
> also adjust the buckets (q->flows_cnt) for this not to break. (Just
> adjust it to 128)
>
> Problematic OpenWRT commit in question:
>  http://git.openwrt.org/?p=openwrt.git;a=patch;h=12cd6578084e
>  12cd6578084e ("kernel: revert fq_codel quantum override to prevent it from 
> causing too much cpu load with higher speed (#21326)")
I 'pull requested' this to the lede-staging tree on github.
https://github.com/lede-project/staging/pull/11

One way or another Felix & co should see the change :-)
>
>
> I also highly recommend you cherry-pick this very recent commit:
>  net-next: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()")
>  https://git.kernel.org/davem/net-next/c/9d18562a227
>
> This should fix very high CPU usage in-case fq_codel goes into drop mode.
> The problem is that drop mode was considered rare, and implementation
> wise it was chosen to be more expensive (to save cycles on normal mode).
> Unfortunately is it easy to trigger with an UDP flood. Drop mode is
> especially expensive for smaller devices, as it scans a 4K big array,
> thus 64 cache misses for small devices!
>
> The fix is to allow drop-mode to bulk-drop more packets when entering
> drop-mode (default 64 bulk drop).  That way we don't suddenly
> experience a significantly higher processing cost per packet, but
> instead can amortize this.
I haven't done the above cherry-pick patch & backport patch creation for
4.4/4.1/3.18 yet - maybe if $dayjob permits time and no one else beats
me to it :-)

Kevin



smime.p7s
Description: S/MIME Cryptographic Signature