Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading
On 2020-06-24 8:34 p.m., Po Liu wrote:
-Original Message-
That is the point i was trying to get to. Basically:
You have a counter table which is referenced by "index"
You also have a meter/policer table which is referenced by "index".
They should be one same group and same meaning.
Didnt follow. You mean the index is the same for both the
stat and policer?
For policers, they maintain their own stats. So when i say:
tc ... flower ... action police ... index 5 The index referred to is in the
policer table
Sure. Means police with No. 5 entry.
But for other actions, example when i say:
tc ... flower ... action drop index 10
Still the question, does gact action drop could bind with index? It doesn't
meanful.
Depends on your hardware. From this discussion i am
trying to understand where the constraint is for your case.
Whether it is your h/w or the TSN spec.
For a sample counting which is flexible see here:
https://p4.org/p4-spec/docs/PSA.html#sec-counters
That concept is not specific to P4 but rather to
newer flow-based hardware.
More context:
The assumption these days is we can have a _lot_ of flows with a lot
of actions.
Then you want to be able to collect the stats separately, possibly one
counter entry for each action of interest.
Why is this important?f For analytics uses cases,
when you are retrieving the stats you want to reduce the amount of
data being retrieved. Typically these stats are polled every X seconds.
For starters, you dont dump filters (which in your case seems to be
the only way to get the stats).
In current tc, you dump the actions. But that could be improved so
you can just dump the stats. The mapping of stats index to actions
is known to the entity doing the dump.
Does that make sense?
The index is in the counter/stats table.
It is not exactly "10" in hardware, the driver magically hides it from the
user - so it could be hw counter index 1234
Not exactly. Current flower offloading stats means get the chain index for that
flow filter. The other actions should bind to that chain index.
>
So if i read correctly: You have an index per filter pointing to the
counter table.
Is this something _you_ decided to do in software or is it how the
hardware works? (note i referred to this as "legacy ACL" approach
earlier. It worked like that in old hardware because the main use
case was to have one action on a match (drop/accept kind).
Like IEEE802.1Qci, what I am doing is bind gate action to filter
chain(mandatory). And also police action as optional.
I cant seem to find this spec online. Is it freely available?
Also, if i understand you correctly you are saying according to this
spec you can only have the following type of policy:
tc .. filter match-spec-here .. \
action gate gate-action-attributes \
action police ...
That "action gate" MUST always be present
but "action police" is optional?
There is stream counter table which summary the counters pass gate action entry
and police action entry for that chain index(there is a bit different if two
chain sharing same action list).
One chain counter which tc show stats get counter source:
struct psfp_streamfilter_counters {
u64 matching_frames_count;
u64 passing_frames_count;
u64 not_passing_frames_count;
u64 passing_sdu_count;
u64 not_passing_sdu_count;
u64 red_frames_count;
};
Assuming psfp is something defined in IEEE802.1Qci and the spec will
describe these?
Is the filter "index" pointing to one of those in some counter table?
When pass to the user space, summarize as:
stats.pkts = counters.matching_frames_count +
counters.not_passing_sdu_count - filter->stats.pkts;
>
stats.drops = counters.not_passing_frames_count +
counters.not_passing_sdu_count + counters.red_frames_count -
filter->stats.drops;
Thanks for the explanation.
What is filter->stats?
The rest of those counters seem related to the gate action.
How do you account for policing actions?
cheers,
jamal
Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading
On 2020-06-23 7:52 p.m., Po Liu wrote: Hi Jamal, My question: Is this any different from how stats are structured? [..] My question: Why cant you apply the same semantics for the counters? Does your hardware have an indexed counter/stats table? If yes then you Yes, That is the point i was trying to get to. Basically: You have a counter table which is referenced by "index" You also have a meter/policer table which is referenced by "index". For policers, they maintain their own stats. So when i say: tc ... flower ... action police ... index 5 The index referred to is in the policer table But for other actions, example when i say: tc ... flower ... action drop index 10 The index is in the counter/stats table. It is not exactly "10" in hardware, the driver magically hides it from the user - so it could be hw counter index 1234 The old approach is to assume the classifier (flower in this case) has a counter. The reason for this assumption is older hardware was designed to deal with a single action per match. So a counter to the filter is also the counter to the (single) action. I get the feeling your hardware fits in that space. Modern use cases have evolved from the ACL single match and action approach. Maintaining the old thought/architecture breaks in two use cases: 1) when you have multiple actions per policy filter. You need counter-per-action for various reasons 2) Sharing of counters across filters and action. This can be achieve tc supports the above and is sufficient to cover the old use cases. I am just worried, architecturally, we are restricting ourselves to the old scheme. Another reason this is important is for the sake of analytics. A user space app can poll just for the stats table in hardware (or the cached version in the kernel) and reduce the amount of data crossing to user space.. cheers, jamal
Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading
On 2020-06-23 7:55 a.m., Po Liu wrote: [..] My question: Is this any different from how stats are structured? I don't know I fully catch the question. Are you trying to get how many frames for each filter chain passing one index policing action? If one index police action bind to multiple tc filter(they should have differnt chain index ). All those filter should get same index police action stats value since they are sharing the same hardware entry. But I don't think this is the problem. This is a good thing. What is nice is i can use the same index for s/w and h/w (and no need for a translation/remapping). With index provide to device driver(map the s/w action index to a h/w table index ), user could list the police actions list by command: # tc actions show action police Shows the police action table by index. This is also nice. My question: Why cant you apply the same semantics for the counters? Does your hardware have an indexed counter/stats table? If yes then you should be able to do similar thing for counters as you do for policer (i.e use an index and share counters across actions). So when i say: tc action drop index 5 and tc action ok index 5 infact they use the same counter. cheers, jamal
Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading
This certainly brings an interesting point which i brought up earlier
when Jiri was doing offloading of stats.
In this case the action index is being used as the offloaded
policer index (note: there'd need to be a check whether the
index is infact acceptable to the h/w etc unless there
2^32 meters available in the hardware).
My question: Is this any different from how stats are structured?
In this case you can map the s/w action index to a h/w table index
(of meters).
My comment then was: hardware i have encountered (and i pointed to P4
model as well) assumes an indexed table of stats.
cheers,
jamal
On 2020-06-23 2:34 a.m., Po Liu wrote:
From: Po Liu
Hardware may own many entries for police flow. So that make one(or
multi) flow to be policed by one hardware entry. This patch add the
police action index provide to the driver side make it mapping the
driver hardware entry index.
Signed-off-by: Po Liu
---
include/net/flow_offload.h | 1 +
net/sched/cls_api.c| 1 +
2 files changed, 2 insertions(+)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index c2ef19c6b27d..eed98075b1ae 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -232,6 +232,7 @@ struct flow_action_entry {
booltruncate;
} sample;
struct {/* FLOW_ACTION_POLICE */
+ u32 index;
s64 burst;
u64 rate_bytes_ps;
u32 mtu;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6aba7d5ba1ec..fdc4c89ca1fa 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3659,6 +3659,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
entry->police.rate_bytes_ps =
tcf_police_rate_bytes_ps(act);
entry->police.mtu = tcf_police_tcfp_mtu(act);
+ entry->police.index = act->tcfa_index;
} else if (is_tcf_ct(act)) {
entry->id = FLOW_ACTION_CT;
entry->ct.action = tcf_ct_action(act);
CONGRATULATIONS
-- CONGRATULATIONS DEAR BENEFICIARY, We the management and staff of SHELL GLOBAL COMPANY PLC have your email address as a winner in our annual year email balloting promotion and We credited your total winning into ATM VISA CARD for your personal use so that no one can have access to it, we have deposited your automated teller machine (atm) visa card of €750,000.00 in the custody of the Security Company which we kept for you for security reasons due to we were unable to reach you since then. Therefore contact the management of the security company below and ask him to advise you on the delivery procedure. NOTE: We have paid them fee for the delivery of your ATM CARD, the only fee you are required to pay them is only their security keeping fee which they said must be paid by you upon your readiness to receive your ATM Card, therefore, contact them and ask them to direct you further on how your ATM CARD can be delivered to you. Here is the contact of the security company below, contact them with your receiving address and telephone number Company name: Sure Security Services Company LTD Manager Name: Mr. Tom Nelson Email: sur...@contractor.net Best Regards, Mr. Bin Jamal SHELL GLOBAL COMPANY P.R.O
Re: [PATCH net-next 0/2] Change tc action identifiers to be more consistent
On 2019-02-07 2:45 a.m., Eli Cohen wrote: This two patch series modifies TC actions identifiers to be more consistent and also puts them in one place so new identifiers numbers can be chosen more easily. For the series: Acked-by: Jamal Hadi Salim cheers, jamal
Re: [PATCH/RFC] Make perf_event_open() propagate errors for use in bpf_perf_event_open()
On 2019-01-11 10:55 a.m., Arnaldo Carvalho de Melo wrote: Hi Peter, bpf_perf_event_open() already returns a value, but if perf_event_output's output_begin (mostly perf_output_begin) fails, the only way to know about that is looking before/after the rb->lost, right? For ring buffer users that is ok, we'll get a PERF_RECORD_LOST, etc, but for BPF programs it would be convenient to get that -ENOSPC and do some fallback, whatever makes sense, like in my augmented_syscalls stuff for 'perf trace', i.e. don't augment it (i.e. push stuff at the end of the normal payload), just don't filter the raw_syscalls:sys_enter, 'perf trace' will get the enter syscall enter event without the pointer dereference at the end, etc, warn the user but don't lose a syscall in the strace-like output. What do you think? Am I missing something? Probably ;-) Ah, its just test built. Works as advertised ;-> Tested-by: Jamal Hadi Salim cheers, jamal
Re: linux-next: manual merge of the net-next tree with the net tree
On 2018-10-08 9:21 p.m., Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the net-next tree got a conflict in:
net/sched/cls_u32.c
between commit:
6d4c407744dd ("net: sched: cls_u32: fix hnode refcounting")
from the net tree and commit:
a030598690c6 ("net: sched: cls_u32: simplify the hell out u32_delete() emptiness
check")
from the net-next tree.
I fixed it up (I reverted the net tree commit as I could not tell
wich parts of it, if any, are still needed) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
Attached should fix it. Al, please double check.
cheers,
jamal
--- a/net-next/net/sched/cls_u32.c 2018-10-09 05:18:04.046642676 -0400
+++ b/net/sched/cls_u32.c 2018-10-09 05:29:51.965528032 -0400
@@ -391,6 +391,7 @@
RCU_INIT_POINTER(root_ht->next, tp_c->hlist);
rcu_assign_pointer(tp_c->hlist, root_ht);
+ root_ht->refcnt++;
rcu_assign_pointer(tp->root, root_ht);
tp->data = tp_c;
return 0;
@@ -606,7 +607,7 @@
struct tc_u_hnode __rcu **hn;
struct tc_u_hnode *phn;
- WARN_ON(ht->refcnt);
+ WARN_ON(--ht->refcnt);
u32_clear_hnode(tp, ht, extack);
@@ -634,7 +635,7 @@
WARN_ON(root_ht == NULL);
- if (root_ht && --root_ht->refcnt == 0)
+ if (root_ht && --root_ht->refcnt == 1)
u32_destroy_hnode(tp, root_ht, extack);
if (--tp_c->refcnt == 0) {
@@ -679,7 +680,6 @@
}
if (ht->refcnt == 1) {
- ht->refcnt--;
u32_destroy_hnode(tp, ht, extack);
} else {
NL_SET_ERR_MSG_MOD(extack, "Can not delete in-use filter");
@@ -1079,8 +1079,7 @@
}
#endif
- err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], ovr,
- extack);
+ err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], ovr, extack);
if (err == 0) {
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
Re: [PATCH net-next v2] net: sched: don't disable bh when accessing action idr
On 21/05/18 04:03 PM, Vlad Buslov wrote:
Initial net_device implementation used ingress_lock spinlock to synchronize
ingress path of device. This lock was used in both process and bh context.
In some code paths action map lock was obtained while holding ingress_lock.
Commit e1e992e52faa ("[NET_SCHED] protect action config/dump from irqs")
modified actions to always disable bh, while using action map lock, in
order to prevent deadlock on ingress_lock in softirq. This lock was removed
from net_device, so disabling bh, while accessing action map, is no longer
necessary.
Replace all action idr spinlock usage with regular calls that do not
disable bh.
Signed-off-by: Vlad Buslov
Acked-by: Jamal Hadi Salim
cheers,
jamal
Re: [PATCH 00/14] Modify action API for implementing lockless actions
On 14/05/18 04:46 PM, Vlad Buslov wrote: On Mon 14 May 2018 at 18:03, Jamal Hadi Salim wrote: On 14/05/18 10:27 AM, Vlad Buslov wrote: Hello Jamal, I'm trying to run tdc, but keep getting following error even on clean branch without my patches: Vlad, not sure if you saw my email: Apply Roman's patch and try again https://marc.info/?l=linux-netdev&m=152639369112020&w=2 cheers, jamal
Re: [PATCH 00/14] Modify action API for implementing lockless actions
On 14/05/18 10:27 AM, Vlad Buslov wrote: Currently, all netlink protocol handlers for updating rules, actions and qdiscs are protected with single global rtnl lock which removes any possibility for parallelism. This patch set is a first step to remove rtnl lock dependency from TC rules update path. It updates act API to use atomic operations, rcu and spinlocks for fine-grained locking. It also extend API with functions that are needed to update existing actions for parallel execution. Outline of changes: - Change tc action to use atomic reference and bind counters, rcu mechanism for cookie update. - Extend action ops API with 'delete' function and 'unlocked' flag. - Change action API to work with actions in lockless manner based on primitives implemented in previous patches. - Extend action API with new functions necessary to implement unlocked actions. Please run all the tdc tests with these changes. This area has almost good test coverage at this point. If you need help just ping me. cheers, jamal
Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update
On 17-10-10 10:33 PM, Manish Kurup wrote: Using a spinlock in the VLAN action causes performance issues when the VLAN action is used on multiple cores. Rewrote the VLAN action to use RCU read locking for reads and updates instead. Signed-off-by: Manish Kurup Acked-by: Jamal Hadi Salim cheers, jamal
Re: [PATCH] net/sched: act_skbmod: remove unneeded rcu_read_unlock in tcf_skbmod_dump
On 17-03-04 07:01 PM, Alexey Khoroshilov wrote: Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov Acked-by: Jamal Hadi Salim cheers, jamal
Re: [PATCH]net:sched:release lock before tcf_dump_walker() normal return to avoid deadlock
On 16-12-06 12:36 AM, Feng Deng wrote: From: Feng Deng release lock before tcf_dump_walker() normal return to avoid deadlock /Scratching my head. I am probably missing something obvious. What are the condition under which this deadlock will happen? Do you have a testcase we can try? cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 08:45 AM, Cyrill Gorcunov wrote: Note: inet_diag somewhere has a netlink structure that has a hole. I pointed it out to Eric D. and he said we cant add it now because it would break ABI. Naming holes generated by a compiler for alignment sake should not break abi (because alignments are abi by self), but I may be missing something in context of where the structure you're talking about is present. And what about non-x86 archs? They might have different members alignment requirements. struct tcp_info. Sorry - i didnt mean to drag this for long; but the more i think about it, the union is a pragmatic approach. cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 08:27 AM, Jamal Hadi Salim wrote: On 16-09-28 08:16 AM, David Miller wrote: From: Jamal Hadi Salim Date: Wed, 28 Sep 2016 08:09:28 -0400 On 16-09-28 08:07 AM, David Miller wrote: Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake. I think we need to start labelling any new pad fields added as "Not UAPI. Do not fsck fondle this". They must initialize it to zero. What if in the future actually meant to use 0 for something?;-> For example in Cyrill's case it means PROTO_IP Not sure if it useful to interpret or not but it is part of the enum for protocols. I just tested with: socket(AF_INET, SOCK_RAW, 0); and got back: EPROTONOSUPPORT So in this case doesnt matter. But my point stands i.e 0 could mean something. cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 08:16 AM, David Miller wrote: From: Jamal Hadi Salim Date: Wed, 28 Sep 2016 08:09:28 -0400 On 16-09-28 08:07 AM, David Miller wrote: Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake. I think we need to start labelling any new pad fields added as "Not UAPI. Do not fsck fondle this". They must initialize it to zero. What if in the future actually meant to use 0 for something?;-> For example in Cyrill's case it means PROTO_IP Not sure if it useful to interpret or not but it is part of the enum for protocols. Maybe we shouldnt be adding pad fields in these netlink structure definitions then one can liberally add new ones. Note: inet_diag somewhere has a netlink structure that has a hole. I pointed it out to Eric D. and he said we cant add it now because it would break ABI. So where do we draw the line for future extensions? cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 08:07 AM, David Miller wrote: Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake. I think we need to start labelling any new pad fields added as "Not UAPI. Do not fsck fondle this". cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 07:27 AM, Cyrill Gorcunov wrote: On Wed, Sep 28, 2016 at 07:06:26AM -0400, Jamal Hadi Salim wrote: This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch. I am conflicted. A field labelled "pad" does not appear to be valid as "UAPI". It is a cosmetic indicator. If you did sizeof() with or without it being present the value doesnt change. I think you miss the point what I'm trying to say: currently end-user may have reference to this member (for any reason) and his program will compile and run. If we change the name the compilation procedure fails and this will break API. Yes, referrning @pad is bad idea for userspace code, and yes (!) better to simply rename it but lets do that later, on top, so that if we break something in userspace we could easily revert the oneline change. I understood well your point;-> Maybe my response was not clear: _nobody should be fscking fondling pad fields_ setting them or otherwise. Maybe let these programs fail. I asked if you knew any such app which did anything with a pad field. cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 06:51 AM, Cyrill Gorcunov wrote: On Wed, Sep 28, 2016 at 06:43:01AM -0400, Jamal Hadi Salim wrote: [..] I dont know how compilation will fail but you may be right with note: that is not how pads have been used in the past. They are supposed to cosmetic annotation which indicates "here's a hole; use it in the future if you are looking to add something". And someone in the future can claim them. I am not sure if MBZ philosophy applies. This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch. I am conflicted. A field labelled "pad" does not appear to be valid as "UAPI". It is a cosmetic indicator. If you did sizeof() with or without it being present the value doesnt change. BTW: There is at least one major structure in inet diag has a hole today and doesnt have a padding indicator. If protocol goes over u8 then complete inet_diag_req_v2 structure will have to be reworked becaue @sdiag_protocol is u8 as well. IOW, once someone liftup IPPROTO_MAX > 255, he will notice the problem immediately because diag for such module simply stop working properly. ok. cheers, jamal
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 06:17 AM, Cyrill Gorcunov wrote:
On Wed, Sep 28, 2016 at 06:08:00AM -0400, Jamal Hadi Salim wrote:
...
@@ -38,7 +38,10 @@ struct inet_diag_req_v2 {
__u8sdiag_family;
__u8sdiag_protocol;
__u8idiag_ext;
- __u8pad;
+ union {
+ __u8pad;
+ __u8sdiag_raw_protocol; /* SOCK_RAW only, @pad for
others */
+ };
Above looks funny. Why is it a union? pad is for exposing a byte-hole
for padding/alignment reasons and i doubt anybody is using it.
Someone may have set it to zero explicitly on source level, and the
compilation will fail on new kernel then. So no, keeping the name
is reasonable.
I dont know how compilation will fail but you may be right with note:
that is not how pads have been used in the past. They are supposed to
cosmetic annotation which indicates "here's a hole; use it in the
future if you are looking to add something". And someone in the
future can claim them. I am not sure if MBZ philosophy applies.
Should you not just rename it?
Also I notice when things like __raw_v4_lookup() are claiming it is unsigned
short instead of a u8?
The protocol is still up to 255 for a while, is it expected that IPPROTO_MAX
will be increased in more-less near future? Of course I can drop the idea
of using @pad here and switch to some extended reauest but prefer to stick
with simplier solution. Hm?
Ok. If i understood correctly it was already unsigned short before your
patch -so i agree it doesnt matter. Maybe just put a comment to express
that if ever protocol goes above 255 it wont be sufficient.
cheers,
jamal
PS:- sorry for butting in the discussion - i blame it on coffee.
Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets
On 16-09-28 05:03 AM, Cyrill Gorcunov wrote:
In criu we are actively using diag interface to collect sockets
present in the system when dumping applications. And while for
unix, tcp, udp[lite], packet, netlink it works as expected,
the raw sockets do not have. Thus add it.
v2:
- add missing sock_put calls in raw_diag_dump_one (by eric.dumazet@)
- implement @destroy for diag requests (by dsa@)
v3:
- add export of raw_abort for IPv6 (by dsa@)
- pass net-admin flag into inet_sk_diag_fill due to
changes in net-next branch (by dsa@)
v4:
- use @pad in struct inet_diag_req_v2 for raw socket
protocol specification: raw module carries sockets
which may have custom protocol passed from socket()
syscall and sole @sdiag_protocol is not enough to
match underlied ones
- start reporting protocol specifed in socket() call
when sockets are raw ones for the same reason: user
space tools like ss may parse this attribute and use
it for socket matching
v5 (by eric.dumazet@):
- use sock_hold in raw_sock_get instead of atomic_inc,
we're holding (raw_v4_hashinfo|raw_v6_hashinfo)->lock
when looking up so counter won't be zero here.
CC: David S. Miller
CC: Eric Dumazet
CC: David Ahern
CC: Alexey Kuznetsov
CC: James Morris
CC: Hideaki YOSHIFUJI
CC: Patrick McHardy
CC: Andrey Vagin
CC: Stephen Hemminger
Signed-off-by: Cyrill Gorcunov
---
Thanks all for feedback! Take a look please once time permit.
include/net/raw.h |6 +
include/net/rawv6.h|7 +
include/uapi/linux/inet_diag.h |5
net/ipv4/Kconfig |8 +
net/ipv4/Makefile |1
net/ipv4/inet_diag.c |9 +
net/ipv4/raw.c | 21 +++
net/ipv4/raw_diag.c| 233 +
net/ipv6/raw.c |7 -
9 files changed, 292 insertions(+), 5 deletions(-)
Index: linux-ml.git/include/net/raw.h
===
--- linux-ml.git.orig/include/net/raw.h
+++ linux-ml.git/include/net/raw.h
@@ -23,6 +23,12 @@
extern struct proto raw_prot;
+extern struct raw_hashinfo raw_v4_hashinfo;
+struct sock *__raw_v4_lookup(struct net *net, struct sock *sk,
+unsigned short num, __be32 raddr,
+__be32 laddr, int dif);
+
+int raw_abort(struct sock *sk, int err);
void raw_icmp_error(struct sk_buff *, int, u32);
int raw_local_deliver(struct sk_buff *, int);
Index: linux-ml.git/include/net/rawv6.h
===
--- linux-ml.git.orig/include/net/rawv6.h
+++ linux-ml.git/include/net/rawv6.h
@@ -3,6 +3,13 @@
#include
+extern struct raw_hashinfo raw_v6_hashinfo;
+struct sock *__raw_v6_lookup(struct net *net, struct sock *sk,
+unsigned short num, const struct in6_addr
*loc_addr,
+const struct in6_addr *rmt_addr, int dif);
+
+int raw_abort(struct sock *sk, int err);
+
void raw6_icmp_error(struct sk_buff *, int nexthdr,
u8 type, u8 code, int inner_offset, __be32);
bool raw6_local_deliver(struct sk_buff *, int);
Index: linux-ml.git/include/uapi/linux/inet_diag.h
===
--- linux-ml.git.orig/include/uapi/linux/inet_diag.h
+++ linux-ml.git/include/uapi/linux/inet_diag.h
@@ -38,7 +38,10 @@ struct inet_diag_req_v2 {
__u8sdiag_family;
__u8sdiag_protocol;
__u8idiag_ext;
- __u8pad;
+ union {
+ __u8pad;
+ __u8sdiag_raw_protocol; /* SOCK_RAW only, @pad for
others */
+ };
Above looks funny. Why is it a union? pad is for exposing a byte-hole
for padding/alignment reasons and i doubt anybody is using it.
Should you not just rename it?
Also I notice when things like __raw_v4_lookup() are claiming it is
unsigned short instead of a u8?
cheers,
jamal
Re: [BUG] act_ife: sleeping functions called in atomic context
On 16-06-17 01:31 PM, Cong Wang wrote:
My patch is against -net. (I see you already figured out your patch is
missing in -net-next.)
Ok, should have re-read this email before working on the patch;->
Or are you suggesting to rebase it for -net-next? I think it fixes some real
bug so -net is better, although it is slightly large as a bug fix.
I am conflicted. There are a lot of changes in net-next at the moment;
adding this to -net seems like will definetely cause merge issues for
Dave.
Ok, Cong - patch attached and tested. Let me know what you think.
cheers,
jamal
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index b7fa969..b52deb4 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -239,8 +239,7 @@ static int ife_validate_metatype(struct tcf_meta_ops *ops,
void *val, int len)
return ret;
}
-/* called when adding new meta information
- * under ife->tcf_lock
+/* called to find new metadata ops. Possibly load it as a module.
*/
static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
void *val, int len)
@@ -251,11 +250,9 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife,
u32 metaid,
if (!ops) {
ret = -ENOENT;
#ifdef CONFIG_MODULES
- spin_unlock_bh(&ife->tcf_lock);
rtnl_unlock();
request_module("ifemeta%u", metaid);
rtnl_lock();
- spin_lock_bh(&ife->tcf_lock);
ops = find_ife_oplist(metaid);
#endif
}
@@ -272,7 +269,6 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife,
u32 metaid,
}
/* called when adding new meta information
- * under ife->tcf_lock
*/
static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
int len)
@@ -302,7 +298,9 @@ static int add_metainfo(struct tcf_ife_info *ife, u32
metaid, void *metaval,
}
}
+ spin_lock_bh(&ife->tcf_lock);
list_add_tail(&mi->metalist, &ife->metalist);
+ spin_unlock_bh(&ife->tcf_lock);
return ret;
}
@@ -357,7 +355,6 @@ out_nlmsg_trim:
return -1;
}
-/* under ife->tcf_lock */
static void _tcf_ife_cleanup(struct tc_action *a, int bind)
{
struct tcf_ife_info *ife = a->priv;
@@ -385,7 +382,6 @@ static void tcf_ife_cleanup(struct tc_action *a, int bind)
spin_unlock_bh(&ife->tcf_lock);
}
-/* under ife->tcf_lock */
static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb)
{
int len = 0;
@@ -465,6 +461,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
}
ife = to_ife(a);
+ if (exists)
+ spin_lock_bh(&ife->tcf_lock);
ife->flags = parm->flags;
if (parm->flags & IFE_ENCODE) {
@@ -475,10 +473,9 @@ static int tcf_ife_init(struct net *net, struct nlattr
*nla,
saddr = nla_data(tb[TCA_IFE_SMAC]);
}
- spin_lock_bh(&ife->tcf_lock);
ife->tcf_action = parm->action;
-
if (parm->flags & IFE_ENCODE) {
+
if (daddr)
ether_addr_copy(ife->eth_dst, daddr);
else
@@ -492,6 +489,9 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
ife->eth_type = ife_type;
}
+ if (exists)
+ spin_unlock_bh(&ife->tcf_lock);
+
if (ret == ACT_P_CREATED)
INIT_LIST_HEAD(&ife->metalist);
@@ -505,7 +505,6 @@ metadata_parse_err:
if (ret == ACT_P_CREATED)
_tcf_ife_cleanup(a, bind);
- spin_unlock_bh(&ife->tcf_lock);
return err;
}
@@ -524,13 +523,10 @@ metadata_parse_err:
if (ret == ACT_P_CREATED)
_tcf_ife_cleanup(a, bind);
- spin_unlock_bh(&ife->tcf_lock);
return err;
}
}
- spin_unlock_bh(&ife->tcf_lock);
-
if (ret == ACT_P_CREATED)
tcf_hash_insert(tn, a);
Re: [BUG] act_ife: sleeping functions called in atomic context
On 16-06-17 01:38 AM, Cong Wang wrote: On Thu, Jun 16, 2016 at 7:14 PM, Cong Wang wrote: I think we can just remove that tcf_lock, I am testing a patch now. Please try the attached patch, I will do more tests tomorrow. Thanks! Cong, What tree are you using? I dont see the time aggregation patches that I sent (and Dave took in) in your changes. Comments: Is GFP_ATOMIC really necessary? Thats user->kernel interface. GFP_KERNEL should be sufficient. Also, it would be nice to kill the lock - but this feels like two patches in one. 1) to fix the alloc not to be under the lock 2) to kill said lock. Maybe split it as such for easier review. I am using this action extensively so will be happy to test. I think my patch is a good beginning to #1 - if you fix the forgotten unlock and ensure we lock around updating ife fields when it exists already (you said it in your earlier email and I thought about that afterwards). cheers, jamal
Re: [BUG] act_ife: sleeping functions called in atomic context
On 16-06-16 05:43 PM, Cong Wang wrote:
On Thu, Jun 16, 2016 at 1:50 PM, Alexey Khoroshilov
wrote:
tcf_ife_init() contains a big chunk of code executed with
ife->tcf_lock spinlock held. But that code contains several calls
to sleeping functions:
populate_metalist() and use_all_metadata()
-> add_metainfo()
-> find_ife_oplist(metaid)
-> read_lock()
-> try_module_get(o->owner)
-> kzalloc(sizeof(*mi), GFP_KERNEL);
Hmm, we don't need to hold that spinlock when we create a new ife action,
since we haven't inserted it yet. We do need it when we modify an existing
one. So I am thinking if we can refactor that code to avoid spinlock
whenever possible.
Does attached (compile tested) patch help?
-> ops->alloc(mi, metaval);
-> module_put(ops->owner);
_tcf_ife_cleanup()
-> module_put()
The same problem is actual for tcf_ife_cleanup() as well.
Huh? Both module_put() and kfree() should not sleep, right?
I dont think there is any sleeping there ;->
cheers,
jamal
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 6bbc518..e341bef 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -302,7 +302,9 @@ static int add_metainfo(struct tcf_ife_info *ife, u32
metaid, void *metaval,
}
}
+ spin_lock_bh(&ife->tcf_lock);
list_add_tail(&mi->metalist, &ife->metalist);
+ spin_unlock_bh(&ife->tcf_lock);
return ret;
}
@@ -474,7 +476,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
saddr = nla_data(tb[TCA_IFE_SMAC]);
}
- spin_lock_bh(&ife->tcf_lock);
ife->tcf_action = parm->action;
if (parm->flags & IFE_ENCODE) {
@@ -504,7 +505,6 @@ metadata_parse_err:
if (ret == ACT_P_CREATED)
_tcf_ife_cleanup(a, bind);
- spin_unlock_bh(&ife->tcf_lock);
return err;
}
@@ -523,13 +523,10 @@ metadata_parse_err:
if (ret == ACT_P_CREATED)
_tcf_ife_cleanup(a, bind);
- spin_unlock_bh(&ife->tcf_lock);
return err;
}
}
- spin_unlock_bh(&ife->tcf_lock);
-
if (ret == ACT_P_CREATED)
tcf_hash_insert(tn, a);
Re: [PATCH net-next V2] tun: introduce tx skb ring
On 16-06-15 07:52 AM, Jamal Hadi Salim wrote: > On 16-06-15 04:38 AM, Jason Wang wrote: >> We used to queue tx packets in sk_receive_queue, this is less >> efficient since it requires spinlocks to synchronize between producer > > So this is more exercising the skb array improvements. For tun > it would be useful to see general performance numbers on user/kernel > crossing (i.e tun read/write). > If you have the cycles can you run such tests? > Ignore my message - you are running pktgen from a VM towards the host. So the numbers you posted are what i was interested in. Thanks for the good work. cheers, jamal
Re: [PATCH net-next V2] tun: introduce tx skb ring
On 16-06-15 04:38 AM, Jason Wang wrote: > We used to queue tx packets in sk_receive_queue, this is less > efficient since it requires spinlocks to synchronize between producer > and consumer. > > This patch tries to address this by: > > - introduce a new mode which will be only enabled with IFF_TX_ARRAY >set and switch from sk_receive_queue to a fixed size of skb >array with 256 entries in this mode. > - introduce a new proto_ops peek_len which was used for peeking the >skb length. > - implement a tun version of peek_len for vhost_net to use and convert >vhost_net to use peek_len if possible. > > Pktgen test shows about 18% improvement on guest receiving pps for small > buffers: > > Before: ~122pps > After : ~144pps > So this is more exercising the skb array improvements. For tun it would be useful to see general performance numbers on user/kernel crossing (i.e tun read/write). If you have the cycles can you run such tests? cheers, jamal
Re: Deleting child qdisc doesn't reset parent to default qdisc?
On 16-04-14 01:49 PM, Eric Dumazet wrote: And what would be the chosen behavior ? TBF is probably a bad example because it started life as a classless qdisc. There was only one built-in fifo queue that was shaped. Then someone made it classful and changed this behavior. To me it sounds reasonable to have the default behavior restored. At minimal consistency. Relying on TBF installing a bfifo for you at delete would be hazardous. For example CBQ got it differently than HFSC If qdisc_create_dflt() fails in CBQ, we fail the 'delete', while HFSC falls back to noop_qdisc, without warning the user :( At least always using noop_qdisc is consistent. No magic there. Doing 'unification' right now would break existing scripts. This is too late, I am afraid. Sigh. So rant: IMO, we should let any new APIs and API updates stay longer in discussion. Or better mark them as unstable for sometime. The excuse that "it is out in the wild therefore cant be changed" is harmful because the timeline is "forever" whereas patches are applied after a short period of posting and discussions and sometimes not involving the right people. It is like having a jury issuing a death sentence after 1 week of deliberation. You cant take it back after execution. cheers, jamal
Re: [PATCH net v3 2/4] net: add rx_nohandler stat counter
On 16-02-09 03:40 AM, David Miller wrote: From: Eric Dumazet Date: Mon, 08 Feb 2016 14:57:40 -0800 Whole point of TLV is that it allows us to add new fields at the end of the structures. ... Look at iproute2, you were the one adding in 2004 code to cope with various tcp_info sizes. So 12 years later, you cannot say it does not work anymore. +1 The TLV L should be canonical way to determine length. i.e should be sufficient to just look at L and understand that content has changed. But: Using sizeof could be dangerous unless the data is packed to be 32-bit aligned. Looking INET_DIAG_INFO check for sizeof there is a small 8 bit hole in tcp_info I think between these two fields: __u8tcpi_snd_wscale : 4, tcpi_rcv_wscale : 4; __u32 tcpi_rto; --- The kernel will pad to make sure the TLV data is 32-bit aligned. I am not sure if that will be the same length as sizeof() in all hardware + compilers... For this case, it is almost safe to just add a version field - probably in the hole. Or have a #define to say what the expected length should be. Or add an 8 bit pad. In general adding new fields that are non-optional is problematic. i.e by non-optional i mean always expected to be present. I think a good test is old kernel with new iproute2. If the new field is non-optional, it will fail (example iproute2 may try to print a value that it expects but because old kernel doesnt understand it; it is non-existent). cheers, jamal
Re: [PATCH 0/7] Freescale DPAA FMan FLIB(s)
Hi Emil, On 03/05/15 10:04, Emil Medve wrote: Hello Jamal, On 03/05/2015 08:35 AM, Jamal Hadi Salim wrote: Hi Emil, No. All the kernel drivers/code we want to upstream is meant to stand on its own and be used the "normal" Linux/Unix way Ok, thanks - that was my only concern. Note there is a lot of offload efforts going on in Linux right now. Take a look at the proceedings from netdev01. You should take advantage of that to shape the direction of your patches. I have suffered at the hands of the sdk for this processor in the past; it is just a bad idea to keep these SDKs around when Linux can express the features sufficiently. Note: The main thing i'd be interested in is when you get to offloading the classifier/scheduler etc. I know you are focussing on just the ethernet level at the moment, cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] Freescale DPAA FMan FLIB(s)
Hi Emil, On 03/05/15 08:48, Emil Medve wrote: The intent is to upstream the entire suite of the DPAA drivers. All the drivers are still WIP, but B/QMan have been already presented to the upstream community and this is the first attempt to publish (some low level code of) the FMan driver. As we go through our internal checklist and in the same time address community feedback we'll soon get the drivers to be acceptable for the upstream trees The first version of the actual Ethernet driver will follow imminently SDK enablement is a side-effect Meaning? Let me ask the question differently: Do i need your sdk to use the features exposed or can i use something like tc to set up the deficit rr or wred or the exposed classifiers and associated actions? Would your sdk (via user space direct programming) benefit because you have pushed these pieces into the kernel? How are you planning to add support for your classifiers, queue schedulers etc? Yes Yes as in these will be available via linux kernel or via your sdk? Is that a patch on top of this or it is something that sits on user space? Both. Full DPAA/Ethernet enablement will be present in the kernel. We also have support for user-space based approach. I'm unsure where/when we might publish that. Of course the SDK is always a place you can turn to for all the code we have (in whatever state it might be) the sdk is open source? cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] Freescale DPAA FMan FLIB(s)
On 03/05/15 00:45, Emil Medve wrote: From: Igal Liberman The Freescale Data Path Acceleration Architecture (DPAA) is a set of hardware components on specific QorIQ P and T series multicore processors. This architecture provides the infrastructure to support simplified sharing of networking interfaces and accelerators by multiple CPU cores, and the accelerators themselves. One of the DPAA accelerators is the Frame Manager (FMan), which combines the Ethernet network interfaces with packet distribution logic to provide intelligent distribution and queuing decisions for incoming traffic at line rate. This patch presents the FMan Foundation Libraries (FLIB) headers. The FMan FLIB suite adds basic support for the DPAA FMan hardware register access. The FMan FLIB suite is used in Freescale's SDK Releases. Is this intended to merely enable your sdk? How are you planning to add support for your classifiers, queue schedulers etc? Is that a patch on top of this or it is something that sits on user space? cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging: lustre: lustre: lov: lov_dev: fix sparse warning of static declaration
This patch adds a static keyword to cl_lov_device_mutex_class variable
to suppress the warning of static declaration
Signed-off-by: Mohammad Jamal
---
drivers/staging/lustre/lustre/lov/lov_dev.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/lov/lov_dev.c
b/drivers/staging/lustre/lustre/lov/lov_dev.c
index 796a015..711b837 100644
--- a/drivers/staging/lustre/lustre/lov/lov_dev.c
+++ b/drivers/staging/lustre/lustre/lov/lov_dev.c
@@ -60,7 +60,7 @@ struct kmem_cache *lovsub_req_kmem;
struct kmem_cache *lov_lock_link_kmem;
/** Lock class of lov_device::ld_mutex. */
-struct lock_class_key cl_lov_device_mutex_class;
+static struct lock_class_key cl_lov_device_mutex_class;
struct lu_kmem_descr lov_caches[] = {
{
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging: lustre: lnet: lnet: lip-ptl:fix sparse warning of static declaration
This patch adds a static keyword to variable portal_rotor to suppress the sparse warning of static declaration Signed-off-by: Mohammad Jamal --- drivers/staging/lustre/lnet/lnet/lib-ptl.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lnet/lnet/lib-ptl.c b/drivers/staging/lustre/lnet/lnet/lib-ptl.c index 19ed696..6652036 100644 --- a/drivers/staging/lustre/lnet/lnet/lib-ptl.c +++ b/drivers/staging/lustre/lnet/lnet/lib-ptl.c @@ -39,7 +39,7 @@ #include "../../include/linux/lnet/lib-lnet.h" /* NB: add /proc interfaces in upcoming patches */ -intportal_rotor= LNET_PTL_ROTOR_HASH_RT; +static int portal_rotor= LNET_PTL_ROTOR_HASH_RT; module_param(portal_rotor, int, 0644); MODULE_PARM_DESC(portal_rotor, "redirect PUTs to different cpu-partitions"); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging: lustre: lustre: lmv: fix sparse warnings about static declarations
This patch adds a static keyword to lprocfs_lmv_init_vars and
lprocfs_lmv_module_vars to suppress the sparse warnings about
static declaration
Signed-off-by: Mohammad Jamal
---
drivers/staging/lustre/lustre/lmv/lproc_lmv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
b/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
index 5be4176..804476b 100644
--- a/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
+++ b/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
@@ -215,7 +215,7 @@ static struct lprocfs_vars lprocfs_lmv_module_vars[] = {
{ NULL }
};
-struct file_operations lmv_proc_target_fops = {
+static struct file_operations lmv_proc_target_fops = {
.owner = THIS_MODULE,
.open= lmv_target_seq_open,
.read= seq_read,
@@ -223,7 +223,7 @@ struct file_operations lmv_proc_target_fops = {
.release = seq_release,
};
-void lprocfs_lmv_init_vars(struct lprocfs_static_vars *lvars)
+static void lprocfs_lmv_init_vars(struct lprocfs_static_vars *lvars)
{
lvars->module_vars= lprocfs_lmv_module_vars;
lvars->obd_vars = lprocfs_lmv_obd_vars;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging: lustre: lustre: lmv: fix sparse warnings related to static declarations
This patch removes the sparse warnings present in the lproc_lmv.c
Signed-off-by: Mohammad Jamal
---
drivers/staging/lustre/lustre/lmv/lproc_lmv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
b/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
index 5be4176..804476b 100644
--- a/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
+++ b/drivers/staging/lustre/lustre/lmv/lproc_lmv.c
@@ -215,7 +215,7 @@ static struct lprocfs_vars lprocfs_lmv_module_vars[] = {
{ NULL }
};
-struct file_operations lmv_proc_target_fops = {
+static struct file_operations lmv_proc_target_fops = {
.owner = THIS_MODULE,
.open= lmv_target_seq_open,
.read= seq_read,
@@ -223,7 +223,7 @@ struct file_operations lmv_proc_target_fops = {
.release = seq_release,
};
-void lprocfs_lmv_init_vars(struct lprocfs_static_vars *lvars)
+static void lprocfs_lmv_init_vars(struct lprocfs_static_vars *lvars)
{
lvars->module_vars= lprocfs_lmv_module_vars;
lvars->obd_vars = lprocfs_lmv_obd_vars;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH bluetooth-next] ieee802154: cc2520: Replace shift operations by BIT macro
This patch replaces the shifting operations by BIT macro Signed-off-by: Mohammad Jamal --- drivers/net/ieee802154/cc2520.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c index 665a3db..181b349 100644 --- a/drivers/net/ieee802154/cc2520.c +++ b/drivers/net/ieee802154/cc2520.c @@ -44,9 +44,9 @@ #defineCC2520_FREG_MASK0x3F /* status byte values */ -#defineCC2520_STATUS_XOSC32M_STABLE(1 << 7) -#defineCC2520_STATUS_RSSI_VALID(1 << 6) -#defineCC2520_STATUS_TX_UNDERFLOW (1 << 3) +#defineCC2520_STATUS_XOSC32M_STABLEBIT(7) +#defineCC2520_STATUS_RSSI_VALIDBIT(6) +#defineCC2520_STATUS_TX_UNDERFLOW BIT(3) /* IEEE-802.15.4 defined constants (2.4 GHz logical channels) */ #defineCC2520_MINCHANNEL 11 -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH bluetooth-next] ieee802154: cc2520: Fix space before , coding style issue
This patch removes the warnings (space before , ) shown by checkpatch.pl Signed-off-by: Mohammad Jamal --- drivers/net/ieee802154/cc2520.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c index a43c8ac..665a3db 100644 --- a/drivers/net/ieee802154/cc2520.c +++ b/drivers/net/ieee802154/cc2520.c @@ -549,14 +549,14 @@ cc2520_ed(struct ieee802154_hw *hw, u8 *level) u8 rssi; int ret; - ret = cc2520_read_register(priv , CC2520_RSSISTAT, &status); + ret = cc2520_read_register(priv, CC2520_RSSISTAT, &status); if (ret) return ret; if (status != RSSI_VALID) return -EINVAL; - ret = cc2520_read_register(priv , CC2520_RSSI, &rssi); + ret = cc2520_read_register(priv, CC2520_RSSI, &rssi); if (ret) return ret; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: ieee802154: cc2520: Fix coding style issues
On Fri, Jan 23, 2015 at 6:07 PM, Sergei Shtylyov wrote: > Hello. > > On 1/23/2015 12:26 PM, Mohammad Jamal wrote: > >> This patch solves the coding style issues such as space after , > > >s/after/before/? > >> and removes the blank lines > > > Extra blank lines, you mean? > >> Signed-off-by: Mohammad Jamal >> --- >> drivers/net/ieee802154/cc2520.c |6 ++ >> 1 file changed, 2 insertions(+), 4 deletions(-) > > >> diff --git a/drivers/net/ieee802154/cc2520.c >> b/drivers/net/ieee802154/cc2520.c >> index f9df9fa..dd129be 100644 >> --- a/drivers/net/ieee802154/cc2520.c >> +++ b/drivers/net/ieee802154/cc2520.c > > [...] >> >> @@ -551,14 +550,14 @@ cc2520_ed(struct ieee802154_hw *hw, u8 *level) >> u8 rssi; >> int ret; >> >> - ret = cc2520_read_register(priv , CC2520_RSSISTAT, &status); >> + ret = cc2520_read_register(priv, CC2520_RSSISTAT, &status); >> if (ret) >> return ret; >> >> if (status != RSSI_VALID) >> return -EINVAL; >> >> - ret = cc2520_read_register(priv , CC2520_RSSI, &rssi); >> + ret = cc2520_read_register(priv, CC2520_RSSI, &rssi); > > [...] > > WBR, Sergei > Sorry for that, i actually have cloned and linus torvalds git and when i ran checkpatch.pl i have found some warnings. so this patch was removing those warnings...as bhadram told that these warnings are already removed in bluetooth-next tree...so this patch any how fails -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: ieee802154: cc2520: fix coding style issue
On Fri, Jan 23, 2015 at 3:16 PM, Varka Bhadram wrote: > Hi Mohammad Jamal, > > On Fri, Jan 23, 2015 at 3:06 PM, Mohammad Jamal > wrote: >> This patch solves the coding style issue warning >> by replacing the shifting operations by BIT macro >> >> Signed-off-by: Mohammad Jamal >> --- >> drivers/net/ieee802154/cc2520.c |6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ieee802154/cc2520.c >> b/drivers/net/ieee802154/cc2520.c >> index dd129be..b9b2a49 100644 >> --- a/drivers/net/ieee802154/cc2520.c >> +++ b/drivers/net/ieee802154/cc2520.c >> @@ -45,9 +45,9 @@ >> #defineCC2520_FREG_MASK0x3F >> >> /* status byte values */ >> -#defineCC2520_STATUS_XOSC32M_STABLE(1 << 7) >> -#defineCC2520_STATUS_RSSI_VALID(1 << 6) >> -#defineCC2520_STATUS_TX_UNDERFLOW (1 << 3) >> +#defineCC2520_STATUS_XOSC32M_STABLEBIT(7) >> +#defineCC2520_STATUS_RSSI_VALIDBIT(6) >> +#defineCC2520_STATUS_TX_UNDERFLOW BIT(3) >> >> /* IEEE-802.15.4 defined constants (2.4 GHz logical channels) */ >> #defineCC2520_MINCHANNEL 11 >> -- >> 1.7.9.5 >> > > Please work on bluetooth-next[1] tree and also include > 'bluetooth-next' tag for the patch. > > [1]: http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/ > > -- > Thanks and Regards, > Varka Bhadram. OK , i will send u the patch once i clone the bluetooth-next tree. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] net: ieee802154: cc2520: fix coding style issue
This patch solves the coding style issue warning by replacing the shifting operations by BIT macro Signed-off-by: Mohammad Jamal --- drivers/net/ieee802154/cc2520.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c index dd129be..b9b2a49 100644 --- a/drivers/net/ieee802154/cc2520.c +++ b/drivers/net/ieee802154/cc2520.c @@ -45,9 +45,9 @@ #defineCC2520_FREG_MASK0x3F /* status byte values */ -#defineCC2520_STATUS_XOSC32M_STABLE(1 << 7) -#defineCC2520_STATUS_RSSI_VALID(1 << 6) -#defineCC2520_STATUS_TX_UNDERFLOW (1 << 3) +#defineCC2520_STATUS_XOSC32M_STABLEBIT(7) +#defineCC2520_STATUS_RSSI_VALIDBIT(6) +#defineCC2520_STATUS_TX_UNDERFLOW BIT(3) /* IEEE-802.15.4 defined constants (2.4 GHz logical channels) */ #defineCC2520_MINCHANNEL 11 -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] net: ieee802154: cc2520: Fix coding style issues
This patch solves the coding style issues such as space after ,
and removes the blank lines
Signed-off-by: Mohammad Jamal
---
drivers/net/ieee802154/cc2520.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index f9df9fa..dd129be 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -513,7 +513,6 @@ err_tx:
return rc;
}
-
static int cc2520_rx(struct cc2520_private *priv)
{
u8 len = 0, lqi = 0, bytes = 1;
@@ -551,14 +550,14 @@ cc2520_ed(struct ieee802154_hw *hw, u8 *level)
u8 rssi;
int ret;
- ret = cc2520_read_register(priv , CC2520_RSSISTAT, &status);
+ ret = cc2520_read_register(priv, CC2520_RSSISTAT, &status);
if (ret)
return ret;
if (status != RSSI_VALID)
return -EINVAL;
- ret = cc2520_read_register(priv , CC2520_RSSI, &rssi);
+ ret = cc2520_read_register(priv, CC2520_RSSI, &rssi);
if (ret)
return ret;
@@ -947,7 +946,6 @@ static int cc2520_probe(struct spi_device *spi)
if (ret)
goto err_hw_init;
-
gpio_set_value(pdata->vreg, HIGH);
usleep_range(100, 150);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] include:linux:Optimizations to __skb_push
On Thu, Jan 22, 2015 at 11:54 PM, Eric Dumazet wrote: > On Thu, Jan 22, 2015 at 10:21 AM, Tom Herbert wrote: > >> Hmm, this seems less readable to me. What is the effect of this patch? >> Does the compiler even produce different assembly? > > No change at all in generated assembly, this looks not worth it. > > BTW, most __skb_push() callers ignore return value. it just reduces the number of lines -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] include:linux:Optimizations to __skb_push
This patch optimizes __skb_push function
Signed-off-by: Mohammad Jamal
---
include/linux/skbuff.h |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 85ab7d7..9acffb2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1671,9 +1671,8 @@ static inline unsigned char *__skb_put(struct sk_buff
*skb, unsigned int len)
unsigned char *skb_push(struct sk_buff *skb, unsigned int len);
static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
{
- skb->data -= len;
skb->len += len;
- return skb->data;
+ return skb->data -= len;
}
unsigned char *skb_pull(struct sk_buff *skb, unsigned int len);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: gpio-dln2: Added a Blank line after declaration
I think you are write ... checkpatch.pl was giving the error at the line so i added a blank line... i will send an updated patch.. On Thu, Jan 15, 2015 at 11:40 PM, Johan Hovold wrote: > On Thu, Jan 15, 2015 at 06:20:43PM +0100, Linus Walleij wrote: >> On Tue, Jan 13, 2015 at 4:09 PM, Mohammad Jamal >> wrote: >> >> > Fix the coding style issue by adding a blank line after declaration >> > >> > Signed-off-by: Mohammad Jamal >> >> Patch applied. > > This one looks bogus; it's adding a random newline within the > declarations not after. > > Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mmc: host: sdhci: Added a space before (
This patch solves the coding style issue by adding a space before ( Signed-off-by: Mohammad Jamal --- drivers/mmc/host/sdhci.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 1453cd1..910ee7b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1624,7 +1624,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios) * signalling timeout and CRC errors even on CMD0. Resetting * it on each ios seems to solve the problem. */ - if(host->quirks & SDHCI_QUIRK_RESET_CMD_DATA_ON_IOS) + if (host->quirks & SDHCI_QUIRK_RESET_CMD_DATA_ON_IOS) sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA); mmiowb(); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpio: gpio-dln2: Added a Blank line after declaration
Fix the coding style issue by adding a blank line after declaration
Signed-off-by: Mohammad Jamal
---
drivers/gpio/gpio-dln2.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
index ce3c155..dbdb4de 100644
--- a/drivers/gpio/gpio-dln2.c
+++ b/drivers/gpio/gpio-dln2.c
@@ -396,6 +396,7 @@ static void dln2_gpio_event(struct platform_device *pdev,
u16 echo,
const void *data, int len)
{
int pin, irq;
+
const struct {
__le16 count;
__u8 type;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging : iio: meter: ade7759: fix space before , coding style issue
This patch solves the space before , coding style issue found by
checkpatch in ade7759.c
Signed-off-by: Mohammad Jamal
---
drivers/staging/iio/meter/ade7759.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/iio/meter/ade7759.c
b/drivers/staging/iio/meter/ade7759.c
index 7d21743..b0c7dbc 100644
--- a/drivers/staging/iio/meter/ade7759.c
+++ b/drivers/staging/iio/meter/ade7759.c
@@ -116,7 +116,7 @@ static int ade7759_spi_read_reg_40(struct device *dev,
mutex_lock(&st->buf_lock);
st->tx[0] = ADE7759_READ_REG(reg_address);
- memset(&st->tx[1], 0 , 5);
+ memset(&st->tx[1], 0, 5);
ret = spi_sync_transfer(st->us, xfers, ARRAY_SIZE(xfers));
if (ret) {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging: iio: adc: ad7192: fix space before , coding style issue
This patch solves the space before , error of the checkpatch.pl
Signed-off-by: Mohammad Jamal
---
drivers/staging/iio/adc/ad7192.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f6526aa..6f8ce6c 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -612,7 +612,7 @@ static int ad7192_probe(struct spi_device *spi)
const struct ad7192_platform_data *pdata = spi->dev.platform_data;
struct ad7192_state *st;
struct iio_dev *indio_dev;
- int ret , voltage_uv = 0;
+ int ret, voltage_uv = 0;
if (!pdata) {
dev_err(&spi->dev, "no platform data?\n");
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging : gdm724x: Remove space before , in function arguments
This patch solves space prohibited before , warning in gdm_mux.c
Signed-off-by: Mohammad Jamal
---
drivers/staging/gdm724x/gdm_mux.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/gdm724x/gdm_mux.c
b/drivers/staging/gdm724x/gdm_mux.c
index b5b063a..d1ab996 100644
--- a/drivers/staging/gdm724x/gdm_mux.c
+++ b/drivers/staging/gdm724x/gdm_mux.c
@@ -220,7 +220,7 @@ static int up_to_host(struct mux_rx *r)
static void do_rx(struct work_struct *work)
{
struct mux_dev *mux_dev =
- container_of(work, struct mux_dev , work_rx.work);
+ container_of(work, struct mux_dev, work_rx.work);
struct mux_rx *r;
struct rx_cxt *rx = (struct rx_cxt *)&mux_dev->rx;
unsigned long flags;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: clocking-wizard: Added a blank line after declaration
Fix the coding style issue by adding blank line after declaration
Signed-off-by: Mohammad Jamal
---
.../clocking-wizard/clk-xlnx-clock-wizard.c|1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 471d087..ea8d561 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -239,6 +239,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
/* register div per output */
for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
const char *clkout_name;
+
if (of_property_read_string_index(np, "clock-output-names", i,
&clkout_name)) {
dev_err(&pdev->dev,
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mtd: ssfdc: Fix space before ( coding style
Patch solves the space before ( coding style issue of ssfdc.c
Signed-off-by: Mohammad Jamal
---
drivers/mtd/ssfdc.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
index daf82ba..d4a922c 100644
--- a/drivers/mtd/ssfdc.c
+++ b/drivers/mtd/ssfdc.c
@@ -42,8 +42,8 @@ struct ssfdcr_record {
#define MAX_LOGIC_BLK_PER_ZONE 1000
#define MAX_PHYS_BLK_PER_ZONE 1024
-#define KiB(x) ( (x) * 1024L )
-#define MiB(x) ( KiB(x) * 1024L )
+#define KiB(x) ((x) * 1024L)
+#define MiB(x) (KiB(x) * 1024L)
/** CHS Table
1MiB2MiB4MiB8MiB16MiB 32MiB 64MiB 128MiB
@@ -63,13 +63,13 @@ typedef struct {
/* Must be ordered by size */
static const chs_entry_t chs_table[] = {
- { MiB( 1), 125, 4, 4 },
- { MiB( 2), 125, 4, 8 },
- { MiB( 4), 250, 4, 8 },
- { MiB( 8), 250, 4, 16 },
- { MiB( 16), 500, 4, 16 },
- { MiB( 32), 500, 8, 16 },
- { MiB( 64), 500, 8, 32 },
+ { MiB(1), 125, 4, 4 },
+ { MiB(2), 125, 4, 8 },
+ { MiB(4), 250, 4, 8 },
+ { MiB(8), 250, 4, 16 },
+ { MiB(16), 500, 4, 16 },
+ { MiB(32), 500, 8, 16 },
+ { MiB(64), 500, 8, 32 },
{ MiB(128), 500, 16, 32 },
{ 0 },
};
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ad525x_dpot-spi: Added Blank lines after declarations
This patch solves the blank line warning of checkpatch.pl
Signed-off-by: Mohammad Jamal
---
drivers/misc/ad525x_dpot-spi.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/misc/ad525x_dpot-spi.c b/drivers/misc/ad525x_dpot-spi.c
index 9da04ed..f4c82ea 100644
--- a/drivers/misc/ad525x_dpot-spi.c
+++ b/drivers/misc/ad525x_dpot-spi.c
@@ -15,18 +15,21 @@
static int write8(void *client, u8 val)
{
u8 data = val;
+
return spi_write(client, &data, 1);
}
static int write16(void *client, u8 reg, u8 val)
{
u8 data[2] = {reg, val};
+
return spi_write(client, data, 2);
}
static int write24(void *client, u8 reg, u16 val)
{
u8 data[3] = {reg, val >> 8, val};
+
return spi_write(client, data, 3);
}
@@ -34,6 +37,7 @@ static int read8(void *client)
{
int ret;
u8 data;
+
ret = spi_read(client, &data, 1);
if (ret < 0)
return ret;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] ad525x_dpot:Remove break after return
This patch removes the break statements present after return
Signed-off-by: Mohammad Jamal
---
drivers/misc/ad525x_dpot.c |6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/misc/ad525x_dpot.c b/drivers/misc/ad525x_dpot.c
index 32f9072..15e8807 100644
--- a/drivers/misc/ad525x_dpot.c
+++ b/drivers/misc/ad525x_dpot.c
@@ -334,7 +334,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
case DPOT_UID(AD5246_ID):
case DPOT_UID(AD5247_ID):
return dpot_write_d8(dpot, value);
- break;
case DPOT_UID(AD5245_ID):
case DPOT_UID(AD5241_ID):
@@ -346,7 +345,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl = ((reg & DPOT_RDAC_MASK) == DPOT_RDAC0) ?
0 : DPOT_AD5282_RDAC_AB;
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5171_ID):
case DPOT_UID(AD5273_ID):
if (reg & DPOT_ADDR_OTP) {
@@ -356,7 +354,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl = DPOT_AD5273_FUSE;
}
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5172_ID):
case DPOT_UID(AD5173_ID):
ctrl = ((reg & DPOT_RDAC_MASK) == DPOT_RDAC0) ?
@@ -368,7 +365,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl |= DPOT_AD5170_2_3_FUSE;
}
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5170_ID):
if (reg & DPOT_ADDR_OTP) {
tmp = dpot_read_r8d16(dpot, tmp);
@@ -377,7 +373,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl = DPOT_AD5170_2_3_FUSE;
}
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5272_ID):
case DPOT_UID(AD5274_ID):
dpot_write_r8d8(dpot, DPOT_AD5270_1_2_4_CTRLREG << 2,
@@ -392,7 +387,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
return dpot_write_r8d8(dpot, (DPOT_AD5270_1_2_4_RDAC << 2) |
(value >> 8), value & 0xFF);
- break;
default:
if (reg & DPOT_ADDR_CMD)
return dpot_write_d8(dpot, reg);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] ad525x_dpot: Add a blank line after declaration
Added a blank line after declaration to fix the warning of checkpatch.pl
Signed-off-by: Mohammad Jamal
---
drivers/misc/ad525x_dpot.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/misc/ad525x_dpot.c b/drivers/misc/ad525x_dpot.c
index a43053d..32f9072 100644
--- a/drivers/misc/ad525x_dpot.c
+++ b/drivers/misc/ad525x_dpot.c
@@ -176,6 +176,7 @@ static s32 dpot_read_i2c(struct dpot_data *dpot, u8 reg)
{
int value;
unsigned ctrl = 0;
+
switch (dpot->uid) {
case DPOT_UID(AD5246_ID):
case DPOT_UID(AD5247_ID):
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] misc: ad52x_dpot: fix break is not useful after a goto or return coding style issue in ad525x_dpot.c
This is a patch to the ad525x_dpot.c that fixes up the break after return
warning coding style issue found by the checkpatch.pl tool
Signed-off-by: Mohammad Jamal
---
drivers/misc/ad525x_dpot.c |6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/misc/ad525x_dpot.c b/drivers/misc/ad525x_dpot.c
index 32f9072..15e8807 100644
--- a/drivers/misc/ad525x_dpot.c
+++ b/drivers/misc/ad525x_dpot.c
@@ -334,7 +334,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
case DPOT_UID(AD5246_ID):
case DPOT_UID(AD5247_ID):
return dpot_write_d8(dpot, value);
- break;
case DPOT_UID(AD5245_ID):
case DPOT_UID(AD5241_ID):
@@ -346,7 +345,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl = ((reg & DPOT_RDAC_MASK) == DPOT_RDAC0) ?
0 : DPOT_AD5282_RDAC_AB;
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5171_ID):
case DPOT_UID(AD5273_ID):
if (reg & DPOT_ADDR_OTP) {
@@ -356,7 +354,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl = DPOT_AD5273_FUSE;
}
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5172_ID):
case DPOT_UID(AD5173_ID):
ctrl = ((reg & DPOT_RDAC_MASK) == DPOT_RDAC0) ?
@@ -368,7 +365,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl |= DPOT_AD5170_2_3_FUSE;
}
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5170_ID):
if (reg & DPOT_ADDR_OTP) {
tmp = dpot_read_r8d16(dpot, tmp);
@@ -377,7 +373,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
ctrl = DPOT_AD5170_2_3_FUSE;
}
return dpot_write_r8d8(dpot, ctrl, value);
- break;
case DPOT_UID(AD5272_ID):
case DPOT_UID(AD5274_ID):
dpot_write_r8d8(dpot, DPOT_AD5270_1_2_4_CTRLREG << 2,
@@ -392,7 +387,6 @@ static s32 dpot_write_i2c(struct dpot_data *dpot, u8 reg,
u16 value)
return dpot_write_r8d8(dpot, (DPOT_AD5270_1_2_4_RDAC << 2) |
(value >> 8), value & 0xFF);
- break;
default:
if (reg & DPOT_ADDR_CMD)
return dpot_write_d8(dpot, reg);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] misc:ad525x_dpot: Fix missing blank after declaration coding style issue in ad525x_dpot.c
This is a patch to ad525x_dpot.c file that fixes up a missing blank line after
declaration warning found by checkpatch.pl issue
Signed-off-by: Mohammad Jamal
---
drivers/misc/ad525x_dpot.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/misc/ad525x_dpot.c b/drivers/misc/ad525x_dpot.c
index a43053d..32f9072 100644
--- a/drivers/misc/ad525x_dpot.c
+++ b/drivers/misc/ad525x_dpot.c
@@ -176,6 +176,7 @@ static s32 dpot_read_i2c(struct dpot_data *dpot, u8 reg)
{
int value;
unsigned ctrl = 0;
+
switch (dpot->uid) {
case DPOT_UID(AD5246_ID):
case DPOT_UID(AD5247_ID):
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: rtl8192u : fix space before , coding style issue in r8190_rtl8256.c
This is a patch to r8190_rtl8256.c file that fixes space before , warning found
by checkpatch.pl tool
Signed-off-by: Mohammad Jamal
---
drivers/staging/rtl8192u/r8190_rtl8256.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8192u/r8190_rtl8256.c
b/drivers/staging/rtl8192u/r8190_rtl8256.c
index 45514aa..1868352 100644
--- a/drivers/staging/rtl8192u/r8190_rtl8256.c
+++ b/drivers/staging/rtl8192u/r8190_rtl8256.c
@@ -23,7 +23,7 @@
* Return: NONE
* Note: 8226 support both 20M and 40 MHz
*---*/
-void PHY_SetRF8256Bandwidth(struct net_device *dev , HT_CHANNEL_WIDTH
Bandwidth)
+void PHY_SetRF8256Bandwidth(struct net_device *dev, HT_CHANNEL_WIDTH Bandwidth)
{
u8 eRFPath;
struct r8192_priv *priv = ieee80211_priv(dev);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] net: Add Keystone NetCP ethernet driver support
On 09/09/14 11:19, Santosh Shilimkar wrote: All the documentation is open including packet accelerator offload in ti.com. Very nice. Would you do me a kindness and point to the switch interface documentation (and other ones on that soc)? We got such requests from customers but couldn't support it for Linux. It has been difficult because every chip vendor is trying to do their own thing. Some have huge (fugly) SDKs in user space which make it worse. Thats the struggle we are trying to deal with. Of course none of those vendors want to open up their specs. You present a nice opportunity to not follow that path. We are also looking for such support and any direction are welcome. Your slide deck seems to capture the key topics like L2/IPSEC offload which we are also interested to hear. The slides list the most popular offloads. But not necessarily all known offloads. Just to be clear, your point was about L2 switch offload which the driver don't support at the moment. It might confuse others. The driver doesn't implements anything non-standard. If i understood you correctly: Your initial patches dont intend to expose any offloads - you are just abstracting this as a NIC. I think that is a legit reason. However, the problem is you are also exposing the packet processors and switch offloading in a proprietary way. For a sample of how L2 basic functions like FDB tables are controlled within a NIC - take a look at the Intel NICs. Either that or you hide all the offload interfaces and over time add them (starting with L2 - NICs with L2 are common). cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] net: Add Keystone NetCP ethernet driver support
On 09/08/14 10:41, Santosh Shilimkar wrote: The NetCP plugin module infrastructure use all the standard kernel infrastructure and its very tiny. So i found this manual here: http://www.silica.com/fileadmin/02_Products/Productdetails/Texas_Instruments/SILICA_TI_66AK2E05-ds.pdf Glad there is an open document! There are a couple of ethernet switch chips I can spot there. Can i control those with "bridge" or say "brctl" utilities? I can see the bridge ports are exposed and i should be able to control them via ifconfig or ip link. Thats what "standard kernel infrastructure" means. Magic hidden in a driver is not. Take a look at recent netconf discussion (as well as earlier referenced discussions): http://vger.kernel.org/netconf-nf-offload.pdf Maybe we can help providing you some direction? The problem is it doesnt seem that the offload specs for those other pieces are open? e.g how do i add an entry to the L2 switch? cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/3] net: Add Keystone NetCP ethernet driver
On 08/15/14 11:12, Santosh Shilimkar wrote:
I am curious about these two calls below(netcp_process_one_rx_packet
and netcp_tx_submit_skb):
On tx you seem to be broadcasting to all "sub-modules" and on receive
you seem to be invoking from all as well.
I couldnt find the code for any of the sub-modules but i suspect
things like switching/bridging or ipsec etc could be sub-modules.
If yes, have you thought about integrating these features into
Linux proper instead?
cheers,
jamal
+
+static inline int netcp_process_one_rx_packet(struct netcp_intf *netcp)
+{
+
+ /* Call each of the RX hooks */
+ p_info.skb = skb;
+ p_info.rxtstamp_complete = false;
+ list_for_each_entry(rx_hook, &netcp->rxhook_list_head, list) {
+ int ret;
+
+ ret = rx_hook->hook_rtn(rx_hook->order, rx_hook->hook_data,
+ &p_info);
+ if (unlikely(ret)) {
+ dev_err(netcp->ndev_dev, "RX hook %d failed: %d\n",
+ rx_hook->order, ret);
+ netcp->ndev->stats.rx_errors++;
+ dev_kfree_skb(skb);
+ return 0;
+ }
+ }
..
+ return 0;
+}
[..]
+static inline int netcp_tx_submit_skb(struct netcp_intf *netcp,
+ struct sk_buff *skb,
+ struct knav_dma_desc *desc)
+{
+ struct netcp_tx_pipe *tx_pipe = NULL;
+
+ p_info.netcp = netcp;
+ p_info.skb = skb;
+ p_info.tx_pipe = NULL;
+ p_info.psdata_len = 0;
+ p_info.ts_context = NULL;
+ p_info.txtstamp_complete = NULL;
+ p_info.epib = desc->epib;
+ p_info.psdata = desc->psdata;
+ memset(p_info.epib, 0, KNAV_DMA_NUM_EPIB_WORDS * sizeof(u32));
+
+ /* Find out where to inject the packet for transmission */
+ list_for_each_entry(tx_hook, &netcp->txhook_list_head, list) {
+ ret = tx_hook->hook_rtn(tx_hook->order, tx_hook->hook_data,
+ &p_info);
+ if (unlikely(ret != 0)) {
+ dev_err(netcp->ndev_dev, "TX hook %d rejected the packet
with reason(%d)\n",
+ tx_hook->order, ret);
+ ret = (ret < 0) ? ret : NETDEV_TX_OK;
+ goto out;
+ }
+ }
+
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] xfrm: fix handling of XFRM policies mark and mask.
On 13-02-06 09:39 AM, Emmanuel Thierry wrote: I think you misread the example ! I did ;-> Marks are both 1, masks are different. This case is more complex than a policy with no mark (so mark=0 and mask=0) versus a policy with an exact mark (so mark=1 and mask=0x), and i wanted to know if the algorithm would take these kind of cases into account. Aha. I think this is pushing the envelope a little - are there good use cases for this? certainly you could insert with most exact mask first. No such check is made at the moment. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] xfrm: fix handling of XFRM policies mark and mask.
On 13-02-06 08:53 AM, Emmanuel Thierry wrote: Actually, we didn't think about this problem since we work with priorities, putting the default policy (without a mark) at a minor priority than the marked one. I think priorities are the way to go in cases of ambiguity. Your remark makes clearer the ideas behind the design of XFRM, but this leads to an interesting concern. If on policy insertion, the policy were inserted depending on the accuracy of the mark (the more the mask is specific, the more the mark must be put at the beginning of the list), how would we decide which is the more specific between these ones ? ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir out mark 0x0001 mask 0x0005 ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir out mark 0x0001 mask 0x0003 They look different to me, no? i.e i dont see a conflict - one has mark=5 and the other has mark=3. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] xfrm: fix handling of XFRM policies mark and mask.
Hi Steffen, On 13-02-05 03:12 AM, Steffen Klassert wrote: For example, executing the below commands in that order succeed: ip -6 xfrm policy flush ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir out mark 1 mask 0x ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir out The policy with mark 1 is the first we find. The policy passes the mark check and if the flow matches the selectors, we use this policy. But it fails in the reverse order: ip -6 xfrm policy flush ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir out ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir out mark 1 mask 0x RTNETLINK answers: File exists With this scenario, we would find the policy with mark and mask 0 first. This policy passes the mark check too. So we would use this policy if the flow matches the selectors, but the flow asked for a policy with mark 1. I think the intent Romain is expressing is reasonable and should resolved at insertion time(xfrm_policy_insert()). i.e even though the policy (such as mark=1) is inserted afterwards, at insertion time if it proves it is more specific and not duplicate, it should be inserted ahead of the mark=0. The runtime check will work then. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [net V2] act_mirred: do not drop packets when fails to mirror it
On Thu, 2012-08-16 at 14:44 +0800, Jason Wang wrote: > We drop packet unconditionally when we fail to mirror it. This is not intended > in some cases. Consdier for kvm guest, we may mirror the traffic of the bridge > to a tap device used by a VM. When kernel fails to mirror the packet in > conditions such as when qemu crashes or stop polling the tap, it's hard for > the > management software to detect such condition and clean the the mirroring > before. This would lead all packets to the bridge to be dropped and break the > netowrk of other virtual machines. > > To solve the issue, the patch does not drop packets when kernel fails to > mirror > it, and only drop the redirected packets. > > Signed-off-by: Jason Wang Signed-off-by: Jamal Hadi Salim cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] act_mirred: do not drop packets when fails to mirror it
On Wed, 2012-08-15 at 21:42 +0800, Jason Wang wrote:
>
> I met it actually through the following steps:
>
> - start a kvm guest with tap and make it to be an interface of the bridge
> - mirror the ingress traffic of the bridge to the tap
> - terminate the qemu process, the tap device is then removed
> - all packet goes to bridge would be dropped, so the network of guests
> in the same bridge would be broken
>
Makes sense.
Can you please leave the err check braces i.e
if (err) {
m->tcf_qstats.overlimits++;
if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
retval = TC_ACT_SHOT;
else
retval = m->tcf_action;
} else {
retval = m->tcf_action;
}
Or at least dont use TC_ACT_STOLEN.
cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] act_mirred: do not drop packets when fails to mirror it
On Wed, 2012-08-15 at 17:37 +0800, Jason Wang wrote: > We drop packet unconditionally when we fail to mirror it. This is not intended > in some cases. Hi Jason, Did you actually notice the behavior you described or were you going by the XXX comment I had in the code? cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 15/21] net sched: Pass the skb into change so it can access NETLINK_CB
On Mon, 2012-08-13 at 13:18 -0700, Eric W. Biederman wrote: > From: "Eric W. Biederman" > > cls_flow.c plays with uids and gids. Unless I misread that > code it is possible for classifiers to depend on the specific uid and > gid values. Therefore I need to know the user namespace of the > netlink socket that is installing the packet classifiers. Pass > in the rtnetlink skb so I can access the NETLINK_CB of the passed > packet. In particular I want access to sk_user_ns(NETLINK_CB(in_skb).ssk). > > Pass in not the user namespace but the incomming rtnetlink skb into > the the classifier change routines as that is generally the more useful > parameter. > > Cc: Jamal Hadi Salim > Acked-by: Serge Hallyn > Signed-off-by: Eric W. Biederman Acked-by: Jamal Hadi Salim cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: parallel networking
On Mon, 2007-08-10 at 15:33 -0700, David Miller wrote: > Multiply whatever effect you think you might be able to measure due to > that on your 2 or 4 way system, and multiple it up to 64 cpus or so > for machines I am using. This is where machines are going, and is > going to become the norm. Yes, i keep forgetting that ;-> I need to train my brain to remember that. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: parallel networking
On Mon, 2007-08-10 at 14:11 -0700, David Miller wrote: > The problem is that the packet schedulers want global guarantees > on packet ordering, not flow centric ones. > > That is the issue Jamal is concerned about. indeed, thank you for giving it better wording. > The more I think about it, the more inevitable it seems that we really > might need multiple qdiscs, one for each TX queue, to pull this full > parallelization off. > > But the semantics of that don't smell so nice either. If the user > attaches a new qdisc to "ethN", does it go to all the TX queues, or > what? > > All of the traffic shaping technology deals with the device as a unary > object. It doesn't fit to multi-queue at all. If you let only one CPU at a time access the "xmit path" you solve all the reordering. If you want to be more fine grained you make the serialization point as low as possible in the stack - perhaps in the driver. But I think even what we have today with only one cpu entering the dequeue/scheduler region, _for starters_, is not bad actually ;-> What i am finding (and i can tell you i have been trying hard;->) is that a sufficiently fast cpu doesnt sit in the dequeue area for "too long" (and batching reduces the time spent further). Very quickly there are no more packets for it to dequeue from the qdisc or the driver is stoped and it has to get out of there. If you dont have any interupt tied to a specific cpu then you can have many cpus enter and leave that region all the time. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: parallel networking (was Re: [PATCH 1/4] [NET_SCHED] explict hold dev tx lock)
On Mon, 2007-08-10 at 10:22 -0400, Jeff Garzik wrote: > Any chance the NIC hardware could provide that guarantee? If you can get the scheduling/dequeuing to run on one CPU (as we do today) it should work; alternatively you can totaly bypass the qdisc subystem and go direct to the hardware for devices that are capable and that would work but would require huge changes. My fear is there's a mini-scheduler pieces running on multi cpus which is what i understood as being described. > 8139cp, for example, has two TX DMA rings, with hardcoded > characteristics: one is a high prio q, and one a low prio q. The logic > is pretty simple: empty the high prio q first (potentially starving > low prio q, in worst case). sounds like strict prio scheduling to me which says "if low prio starves so be it" > In terms of overall parallelization, both for TX as well as RX, my gut > feeling is that we want to move towards an MSI-X, multi-core friendly > model where packets are LIKELY to be sent and received by the same set > of [cpus | cores | packages | nodes] that the [userland] processes > dealing with the data. Does putting things in the same core help? But overall i agree with your views. > There are already some primitive NUMA bits in skbuff allocation, but > with modern MSI-X and RX/TX flow hashing we could do a whole lot more, > along the lines of better CPU scheduling decisions, directing flows to > clusters of cpus, and generally doing a better job of maximizing cache > efficiency in a modern multi-thread environment. I think i see the receive with a lot of clarity, i am still foggy on the txmit path mostly because of the qos/scheduling issues. > IMO the current model where each NIC's TX completion and RX processes > are both locked to the same CPU is outmoded in a multi-core world with > modern NICs. :) Infact even with status quo theres a case that can be made to not bind to interupts. In my recent experience with batching, due to the nature of my test app, if i let the interupts float across multiple cpus i benefit. My app runs/binds a thread per CPU and so benefits from having more juice to send more packets per unit of time - something i wouldnt get if i was always running on one cpu. But when i do this i found that just because i have bound a thread to cpu3 doesnt mean that thread will always run on cpu3. If netif_wakeup happens on cpu1, scheduler will put the thread on cpu1 if it is to be run. It made sense to do that, it just took me a while to digest. > But I readily admit general ignorance about the kernel process > scheduling stuff, so my only idea about a starting point was to see how > far to go with the concept of "skb affinity" -- a mask in sk_buff that > is a hint about which cpu(s) on which the NIC should attempt to send and > receive packets. When going through bonding or netfilter, it is trivial > to 'or' together affinity masks. All the various layers of net stack > should attempt to honor the skb affinity, where feasible (requires > interaction with CFS scheduler?). There would be cache benefits if you can free the packet on the same cpu it was allocated; so the idea of skb affinity is useful in the minimal in that sense if you can pull it. Assuming hardware is capable, even if you just tagged it on xmit to say which cpu it was sent out on, and made sure thats where it is freed, that would be a good start. Note: The majority of the packet processing overhead is _still_ the memory subsystem latency; in my tests with batched pktgen improving the xmit subsystem meant the overhead on allocing and freeing the packets went to something > 80%. So something along the lines of parallelizing based on a split of alloc free of sksb IMO on more cpus than where xmit/receive run would see more performance improvements. > Or maybe skb affinity is a dumb idea. I wanted to get people thinking > on the bigger picture. Parallelization starts at the user process. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.23-rc5: possible irq lock inversion dependency detected
On Tue, 2007-11-09 at 10:18 +0800, Herbert Xu wrote:
> Jamal, it's the police_lock that we need to make _bh. The
> ingress_lock is already _bh because of the spin_lock_bh that
> directly precedes it.
>
> Oh and I think the same thing applies for the other actions
> too.
ga-Dang. Ok, here it is. If you see(?) any more farts let me know.
I am around for another 30 minutes and off for about 18 hours.
Christian, i took your config and qos setup but I cant reproduce the
issue - i think i may need some of that wireless setup to recreate. So
if you can test this and validate it works we can push it forward.
cheers,
jamal
[NET_SCHED] protect action config/dump from irqs
>From the sharp laser eyes of Herbert Xu to my slow farting brain...
(with no apologies to C Heston)
On Mon, 2007-10-09 at 21:00 +0800, Herbert Xu wrote:
On Sun, Sep 02, 2007 at 01:11:29PM +, Christian Kujau wrote:
> >
> > after upgrading to 2.6.23-rc5 (and applying davem's fix [0]), lockdep
> > was quite noisy when I tried to shape my external (wireless) interface:
> >
> > [ 6400.534545] FahCore_78.exe/3552 just changed the state of lock:
> > [ 6400.534713] (&dev->ingress_lock){-+..}, at: []
> > netif_receive_skb+0x2d5/0x3c0
> > [ 6400.534941] but this lock took another, soft-read-irq-unsafe lock in the
> > past:
> > [ 6400.535145] (police_lock){-.--}
>
> This is a genuine dead-lock. The police lock can be taken
> for reading with softirqs on. If a second CPU tries to take
> the police lock for writing, while holding the ingress lock,
> then a softirq on the first CPU can dead-lock when it tries
> to get the ingress lock.
Signed-off-by: Jamal Hadi Salim <[EMAIL PROTECTED]>
--- a/net/sched/act_police.c2007/09/11 10:39:36 1.1
+++ b/net/sched/act_police.c2007/09/11 10:51:47
@@ -56,7 +56,7 @@
int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
struct rtattr *r;
- read_lock(&police_lock);
+ read_lock_bh(&police_lock);
s_i = cb->args[0];
@@ -85,7 +85,7 @@
}
}
done:
- read_unlock(&police_lock);
+ read_unlock_bh(&police_lock);
if (n_i)
cb->args[0] += n_i;
return n_i;
--- a/net/sched/act_api.c 2007/09/11 10:47:51 1.1
+++ b/net/sched/act_api.c 2007/09/11 10:50:47
@@ -68,7 +68,7 @@
int err = 0, index = -1,i = 0, s_i = 0, n_i = 0;
struct rtattr *r ;
- read_lock(hinfo->lock);
+ read_lock_bh(hinfo->lock);
s_i = cb->args[0];
@@ -96,7 +96,7 @@
}
}
done:
- read_unlock(hinfo->lock);
+ read_unlock_bh(hinfo->lock);
if (n_i)
cb->args[0] += n_i;
return n_i;
@@ -156,13 +156,13 @@
{
struct tcf_common *p;
- read_lock(hinfo->lock);
+ read_lock_bh(hinfo->lock);
for (p = hinfo->htab[tcf_hash(index, hinfo->hmask)]; p;
p = p->tcfc_next) {
if (p->tcfc_index == index)
break;
}
- read_unlock(hinfo->lock);
+ read_unlock_bh(hinfo->lock);
return p;
}
Re: 2.6.23-rc5: possible irq lock inversion dependency detected
On Mon, 2007-10-09 at 21:00 +0800, Herbert Xu wrote:
> The minimal fix would be to make sure that we disable BH on
> the first CPU.
disabling BH would make it more symmetric to the way we handle
egress. I couldnt reproduce the issue, but this should hopefully resolve
it.
Christian, can you test with this patch?
cheers,
jamal
[NET_SCHED] make ingress qlock symmetric to egress
Signed-off-by: Jamal Hadi Salim <[EMAIL PROTECTED]>
--- a/net/sched/sch_generic.c 2007/09/10 23:19:45 1.1
+++ b/net/sched/sch_generic.c 2007/09/10 23:52:45
@@ -42,12 +42,12 @@
void qdisc_lock_tree(struct net_device *dev)
{
spin_lock_bh(&dev->queue_lock);
- spin_lock(&dev->ingress_lock);
+ spin_lock_bh(&dev->ingress_lock);
}
void qdisc_unlock_tree(struct net_device *dev)
{
- spin_unlock(&dev->ingress_lock);
+ spin_unlock_bh(&dev->ingress_lock);
spin_unlock_bh(&dev->queue_lock);
}
AW: [PATCH] drivers/net/skfp/drvfbi.c: trivial cleanup
Vielen Dank Herr Seidel fuer Ihr Feedback.
Mit freundlichen Gruessen
Jamal
Marvell® Semiconductor Germany GmbH
-
Dipl. -Ing. Karim Jamal
Technical Support Engineer
--
Phone: +49 (0) 7243502-330
Fax: +49 (0) 7243502-364
Mail: [EMAIL PROTECTED]
Web: http:\\www.syskonnect.de
Marvell Semiconductor Germany GmbH, Siemensstr. 23, 76275 Ettlingen,
Amtsgericht Karlsruhe HRB 1620 E Geschäftsführer: Dipl.-Volksw. Mathias Horak,
Thomas Ruf
-Ursprüngliche Nachricht-
Von: Frank Seidel [mailto:[EMAIL PROTECTED]
Gesendet: Montag, 14. Mai 2007 23:28
An: Linux Kernel
Cc: [EMAIL PROTECTED]
Betreff: [PATCH] drivers/net/skfp/drvfbi.c: trivial cleanup
This patch (for 2.6.21-mm2) contains a trivial cleanup:
- removes unused exist_board() variation
- removed part is depecrated and doesn't compile anymore
Signed-off-by: Frank Seidel <[EMAIL PROTECTED]>
---
drivers/net/skfp/drvfbi.c | 40
1 file changed, 40 deletions(-)
Index: b/drivers/net/skfp/drvfbi.c
===
--- a/drivers/net/skfp/drvfbi.c
+++ b/drivers/net/skfp/drvfbi.c
@@ -1224,46 +1224,6 @@ int get_board_para(struct s_smc *smc, in }
#endif /* ISA */
-#ifdef PCI
-#ifdef USE_BIOS_FUN
-int exist_board(struct s_smc *smc, int slot)
-{
- u_short dev_id ;
- u_short ven_id ;
- int found ;
- int i ;
-
- found = FALSE ; /* make sure we returned with adatper not
found*/
- /* if an empty oemids.h was included */
-
-#ifdef MULT_OEM
-smc->hw.oem_id = (struct s_oem_ids *) &oem_ids[0] ;
- for (; smc->hw.oem_id->oi_status != OI_STAT_LAST; smc->hw.oem_id++) {
- if (smc->hw.oem_id->oi_status < smc->hw.oem_min_status)
- continue ;
-#endif
- ven_id = OEMID(smc,0) + (OEMID(smc,1) << 8) ;
- dev_id = OEMID(smc,2) + (OEMID(smc,3) << 8) ;
- for (i = 0; i < slot; i++) {
- if (pci_find_device(i,&smc->hw.pci_handle,
- dev_id,ven_id) != 0) {
-
- found = FALSE ;
- } else {
- found = TRUE ;
- }
- }
- if (found) {
- return(1) ; /* adapter was found */
- }
-#ifdef MULT_OEM
- }
-#endif
- return(0) ; /* adapter was not found */
-}
-#endif /* PCI */
-#endif /* USE_BIOS_FUNC */
-
void driver_get_bia(struct s_smc *smc, struct fddi_addr *bia_addr) {
int i ;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] IPROUTE: Modify tc for new PRIO multiqueue behavior
On Tue, 2007-24-04 at 21:05 -0700, Stephen Hemminger wrote: > Peter P Waskiewicz Jr wrote: > Only if this binary compatiable with older kernels. It is not. But i think that is a lesser problem, the bigger question is: Why would you need to change a qdisc just so you can support egress multiqueues? BTW, is there any reason this is being cced to lkml? cheers, jamal PS:- I havent read the kernel patches (i am congested and about 1000 messages behind on netdev) and my opinions may be influenced by an approach i have in trying to help someone fixup a wireless driver with multiqueue support. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Arp announce (for Xen)
On Fri, 2007-02-03 at 13:54 +0100, Andi Kleen wrote: > Seems like the right solution to me (if it works) I like it as well given the simple change. These are the kind of optimizations that justify offloading things to the kernel (instead of user space) i.e very little lines of code, covers a large number of user-desires, and can be turned off if you want to do something complex. _Almost_ elegant Stephen;-> cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] net/xfrm: fix crash in ipsec audit logging
On Tue, 2006-26-12 at 18:56 +0100, Ingo Molnar wrote:
> + xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
> +AUDIT_MAC_IPSEC_DELSPD, delete, xp, NULL);
> +
> if (!delete) {
> struct sk_buff *resp_skb;
You could move the call into the else from above if (!delete) maybe?
Otherwise you have to add back the "if (delete)" check since that
function could be used to either retrieve (which is not subject to an
audit) or delete an xp.
cheers,
jamal
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [take28-resend_1->0 0/8] kevent: Generic event handling mechanism.
On Thu, 2006-21-12 at 17:46 +0300, Evgeniy Polyakov wrote: > On Thu, Dec 21, 2006 at 09:40:26AM -0500, jamal ([EMAIL PROTECTED]) wrote: > > > Things like sockets/pipes can only benefit from direct kevent usage > > > instead of ->poll() and wrappers. > > > > You should be able change it to use those schemes when it detects > > that the kernel supports them. > > I.e. stat() for each new file descriptor - note, that _you_ asked it :) Didnt follow. Is there some issue with libevent you mean? cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [take28-resend_1->0 0/8] kevent: Generic event handling mechanism.
On Thu, 2006-21-12 at 17:36 +0300, Evgeniy Polyakov wrote: > Btw, it uses only read/write/signal on fd events, so it must use > ->poll() and thus be as fast as epoll. > It is supposed to "detect" the best mechanism in the kernel and switch to that. At the moment for example in my app it defaults to epoll. > Things like sockets/pipes can only benefit from direct kevent usage > instead of ->poll() and wrappers. You should be able change it to use those schemes when it detects that the kernel supports them. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [take28-resend_1->0 0/8] kevent: Generic event handling mechanism.
On Thu, 2006-21-12 at 17:04 +0300, Evgeniy Polyakov wrote: > I modified world-wide used web server lighttpd and ran a lot of tests > with it (compared to epoll version with major performance win). > > I was asked yesterday by Jan Kneschke (lighttpd main developer) if > kevent API is ready so he could include my patch into mainline lighttpd > tree, but I answered 'I do not know if kevent will be or not included, > everyone keeps silence'. > Ok, so you are building the momentum then. People like Jan are the users of such API and should be speaking up. Then kernel people will be forced to look at it. > I just do not know _what_ else should be done not even for inclusion - > but at least for some progress. I know you are frustrated but stop doing the above like a broken vinyl record, it doesnt help your case. > You want libevent to be patched? Its site is currently down, but ok, I > will create a patch. > I promise in 2 weeks to migrate an app or two that i have that use libevent to your version if you have it by then. I will then test and give you my opinion. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [take28-resend_1->0 0/8] kevent: Generic event handling mechanism.
Evgeniy, On Thu, 2006-21-12 at 13:49 +0300, Evgeniy Polyakov wrote: > So comment on its bugs, its design, implementation, ask questions, > request features, show interest (even with 'I have no time right now, > but will loko at it after in a week after vacations'). > > No one does it, so no one cares, so my behaviour. > Please dont be discouraged by lack of attention - you are doing good work. I will concur with Jeff's point that since you are putting out a profound conceptual changes, and there are many stake holders, it requires scrutiny on their part. You need to build consensus in such a situation. Some things that would help progress and build momentum: - As i have advised you before, why dont you modify something like existing libraries such as some of the loop thingies of desktop managers such as kde/gnome or better things like libevent etc. Then write your app on top of that? nobody is gonna run your httpd but if you demonstrate that libevent is much better with your changes (with zero changes to apps), people will migrate - from a user space angle if people like Ulrich would state their views on the current version you have. Note, they dont have to agree with you i.e the conclusion could be a simple "agree to disagree". - There really oughta be a limit on how long people are allowed to be silent. After that IMO your code should just go in ... cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Network drivers that don't suspend on interface down
On Wed, 2006-20-12 at 22:14 -0500, Dan Williams wrote: ... > Simple == good. Down == down. Lets just agree on that and save > ourselves a lot of pain. netdevices have well defined operational and administrative state machines. And very well defined relationship between operational and administrative status. IOW, care should be invoked not to reinvent. Power management to me seems like an operational state. A link could only transition to operational or down depending on whether it is "powered" up or down. To be complete, since a netdevice is a generic construct, nota bene: - a link could be a wireless association or ethernet cable or a PPP session or a ATM PVC, or an infrared channel etc. - events that result in operational link transitions could be anything from powering up an ethernet phy with an active cable plugged to an 802.1x auth on a wireless association to a on-demand ppp link seeing an outgoing packet. IMO, for this discussion to be meaningful, it would be useful to read Documentation/networking/operstates.txt And if you are keen you can then read RFC 2863... cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: ACPI/HT or Packet Scheduler BUG?
On Sat, 2005-16-04 at 13:34 +0200, Thomas Graf wrote: > * Herbert Xu <[EMAIL PROTECTED]> 2005-04-16 21:23 > > On Sat, Apr 16, 2005 at 01:06:39PM +0200, Thomas Graf wrote: > > > > > > It's not completely useless, it speeds up the deletion classful > > > qdiscs having some depth. However, it's not worth the locking > > > troubles I guess. > > > > RCU is meant to optimise the common reader path. In this case > > that's the packet transmission code. Unfortunately it fails > > miserably when judged by that criterion. > > There is one case where it can do good for latency which is for > per flow qdiscs or any other scenarios implying hundreds or > thousands of leaf qdiscs where a destroyage of one such qdisc > tree will take up quite some cpu to traverse all the classes > under dev->queue_lock. I don't have any numbers on this, but > I don't completely dislike the method of hiding the qdiscs under > the lock and do the expensive traveling unlocked. The rule of "optimize for the common" fails miserably in this case because this is not a common case/usage of qdiscs. I have a feeling though that the patch went in due to dude-optimizing-loopback as pointed by Herbert. It could also be it was done because RCU-is-so-cool. I dont recall. Maybe worth reverting to the earlier scheme if it is going to continue to be problematic. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: ACPI/HT or Packet Scheduler BUG?
Didnt see the beginings of this thread - please post on netdev instead
of lkml network related questions.
The real cause seems to be an ARP issue from what i saw in the oops
posted a while back:
--
[4294692.342000] Call Trace:
[4294692.342000] [] show_stack+0xa6/0xe0
[4294692.342000] [] show_registers+0x15b/0x1f0
[4294692.342000] [] die+0x141/0x2d0
[4294692.342000] [] do_page_fault+0x22e/0x6a6
[4294692.342000] [] error_code+0x4f/0x54
[4294692.342000] [] qdisc_restart+0xba/0x730
[4294692.342000] [] dev_queue_xmit+0x13e/0x640
[4294692.342000] [] arp_solicit+0xfc/0x210
[4294692.342000] [] neigh_timer_handler+0x13e/0x320
[4294692.342000] [] run_timer_softirq+0x130/0x490
[4294692.342000] [] __do_softirq+0x42/0xa0
[4294692.342000] [] do_softirq+0x51/0x60
-
Is this the same issue?
Can you describe how you create this issue; kernel version etc.
cheers,
jamal
On Fri, 2005-15-04 at 17:37 -0400, Steven Rostedt wrote:
> On Thu, 2005-04-14 at 18:46 +0300, Tarhon-Onu Victor wrote:
> > On Tue, 12 Apr 2005, Tarhon-Onu Victor wrote:
> >
> > > So the problem should be looked in that changes to the pkt sched API,
> > > the patch containing only those changes is at
> >
> > The bug is in this portion of code from net/sched/sch_generic.c,
> > in the qdisc_destroy() function:
> >
> > ==
> > list_for_each_entry(cq, &cql, list)
> >list_for_each_entry_safe(q, n, &qdisc->dev->qdisc_list, list)
> > if (TC_H_MAJ(q->parent) == TC_H_MAJ(cq->handle)) {
> > if (q->ops->cl_ops == NULL)
> > list_del_init(&q->list);
> > else
> > list_move_tail(&q->list, &cql);
> > }
> > list_for_each_entry_safe(cq, n, &cql, list)
> >list_del_init(&cq->list);
> > ==
> >
> > ...and it happens when q->ops->cl_ops is NULL and
> > list_del_init(&q->list) is executed.
> >
> > The stuff from include/linux/list.h looks ok, it seems like one
> > of those two iterations (list_for_each_entry() and
> > list_for_each_entry_safe()) enters an endless loop when an element is
> > removed from the list under some circumstances.
>
> There's a comment above qdisc_destroy that says:
>
> /* Under dev->queue_lock and BH! */
>
> I'm not so sure this is the case. I've included the emails of those
> listed as Authors of sch_generic.c and sch_htb.c, hopefully they are the
> ones who can help (if not, sorry to bother you).
>
> The list.h is fine, but if another task goes down this list when it
> list_del_init is done, there's a chance that the reading task can get to
> the deleted item just as it is being deleted, and has pointed itself to
> itself. p->next == p. This would go into an infinite loop.
>
> The reason sysrq works is because this doesn't stop interrupts. But put
> a local_irq_save around that list and run your test, I bet you won't be
> able to do anything, but power off with the big button.
>
> Hope someone can help. I don't know the queue disciplines well enough to
> make a proper fix.
>
> -- Steve
>
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]
On Sun, 2005-04-10 at 10:56, James Morris wrote: > On 10 Apr 2005, jamal wrote: > > > Thats what the original motivation for konnector was. To make it easy > > for joe dumbass. > > Who you really want writing kernel code :-) Ok, let me take that back then ;-> The value is in allowing people who are kernel hackers that are trying to focus on an entirely different problem to not really know much about the internals of the messaging subsystem (if you wanna call netlink that). A good example will be the fork patches which were posted recently. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]
Evgeniy, Please crosspost on netdev - you should know that by now;-> I actually disagreee with Herbert on this. Theres definetely good need to have a more usable messaging system that rides on top of netlink. It is not that netlink cant be extended (I actually think thats a separate topic) - its just that its usability curve is too high. Thats what the original motivation for konnector was. To make it easy for joe dumbass. And i think if konnector sticks to just doing that it will end up being valuable. I do think (and ive said this before) that Evgeniy is pushing it by going beyond this simple agenda/focus. Unfortunately, I actually dont think he is listening _at all_ on this specific issue. Evgeniy, just stick to the original focus and if it is accepted and understood then lets move on to adding new features. Otherwise the result of you adding yet one more feature for CBUS or whatever is clearly to question what the original motivation was. And i dont think you are able to add any other points to justify the existence of any new konnector feature other than describe the original goals. At least thats what i saw reading this thread. Otherwise if you really dont know what you want yet lets just pull this whole thing out IMO. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Netlink Connector / CBUS
and, oh yeah - wheres the documentation Evgeniy? ;->
cheers,
jamal
On Tue, 2005-04-05 at 06:44, jamal wrote:
> To be fair to Evgeniy I am not against the Konnector idea. I think that
> it is a useful feature to have an easy to use messaging between
> kernel-kernel and kernel-userspace. The fact that he leveraged netlink
> instead of inventing things is a bonus. Having said that i have not
> seriously scrutinized the code - and i think the idea of this new thing
> hes tossing around called CBUS maybe pushing it.
>
> cheers,
> jamal
>
> On Tue, 2005-04-05 at 03:34, Evgeniy Polyakov wrote:
> > On Tue, 2005-04-05 at 01:10 -0400, Herbert Xu wrote:
> > >On Tue, Apr 05, 2005 at 11:03:16AM +0400, Evgeniy Polyakov wrote:
> > >>
> > >> I received comments and feature requests from Herbert Xu and Jamal Hadi
> > >> Salim,
> > >> almost all were successfully resolved.
> > >
> > >Please do not construe my involvement in these threads as endorsement
> > >for this system.
> >
> > Sure.
> > I remember you are against it :).
> >
> > >In fact to this day I still don't understand what problems this thing is
> > >meant to solve.
> >
> > Hmm, what else can I add to my words?
> > May be checking the size of the code needed to broadcast kobject changes
> > in kobject_uevent.c for example...
> > Netlink socket allocation + skb handling against call to cn_netlink_send().
> >
> > >--
> > >Visit Openswan at http://www.openswan.org/
> > >Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
> > >Home Page: http://gondor.apana.org.au/~herbert/
> > >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> >
>
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Netlink Connector / CBUS
To be fair to Evgeniy I am not against the Konnector idea. I think that
it is a useful feature to have an easy to use messaging between
kernel-kernel and kernel-userspace. The fact that he leveraged netlink
instead of inventing things is a bonus. Having said that i have not
seriously scrutinized the code - and i think the idea of this new thing
hes tossing around called CBUS maybe pushing it.
cheers,
jamal
On Tue, 2005-04-05 at 03:34, Evgeniy Polyakov wrote:
> On Tue, 2005-04-05 at 01:10 -0400, Herbert Xu wrote:
> >On Tue, Apr 05, 2005 at 11:03:16AM +0400, Evgeniy Polyakov wrote:
> >>
> >> I received comments and feature requests from Herbert Xu and Jamal Hadi
> >> Salim,
> >> almost all were successfully resolved.
> >
> >Please do not construe my involvement in these threads as endorsement
> >for this system.
>
> Sure.
> I remember you are against it :).
>
> >In fact to this day I still don't understand what problems this thing is
> >meant to solve.
>
> Hmm, what else can I add to my words?
> May be checking the size of the code needed to broadcast kobject changes
> in kobject_uevent.c for example...
> Netlink socket allocation + skb handling against call to cn_netlink_send().
>
> >--
> >Visit Openswan at http://www.openswan.org/
> >Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
> >Home Page: http://gondor.apana.org.au/~herbert/
> >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH] network configs: disconnect network options from drivers
On Thu, 2005-03-31 at 02:47, Randy.Dunlap wrote: > RFC: This is a work-in-progress (WIP), not yet completed. > > A few people dislike that the Networking Options menu is inside > the Device Drivers/Networking menu. This patch moves the > Networking Options menu to immediately before the Device Drivers menu, > renames it to "Networking options and protocols", & moves most > protocols to more logical places (IMHOOC). > About time someone brave did this. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch] net: fix build break when CONFIG_NET_CLS_ACT is not set
On Tue, 2005-03-29 at 16:17, Neil Horman wrote: > No worries. What exactly is the point of contention on netdev? (I'm not > currently following that list). My patch seems to follow the common practice > for CONFIG_NET_CLS_ACT, in that all references to the action member of the > appropriate struct are themselves ifdef-ed. We are trying to kill appearance of any #ifdef CONFIG_NET_CLS_ACT in the classifiers. The patch you sent is correct except it will introduce an ifdef that we are trying to kill. The current workaround is to turn on CONFIG_NET_CLS_ACT in the kernel build. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch] net: fix build break when CONFIG_NET_CLS_ACT is not set
This is being discussed on netdev at the moment. Thomas Graf is working on a patch. Thanks for the effort though. cheers, jamal On Tue, 2005-03-29 at 15:25, Neil Horman wrote: > Patch to fix build break that occurs when CONFIG_NET_CLS_ACT is not set. > > Signed-off-by: Neil Horman <[EMAIL PROTECTED]> > > cls_fw.c |3 ++- > cls_route.c |3 ++- > cls_tcindex.c |3 ++- > cls_u32.c |2 ++ > 4 files changed, 8 insertions(+), 3 deletions(-) > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak in net/sched/ipt.c?
Actually its well documented even on the man pages - so should have not even have bothered discussing it;-> Should get new brand of coffee. Apologies for unecessarily abused electrons - which means dont respond ;-> cheers, jamal On Wed, 2005-03-23 at 09:05, jamal wrote: > On Wed, 2005-03-23 at 08:40, Jan Engelhardt wrote: > > >I have seen people put little comments of "kfree will work if you > > >pass it NULL" - are you saying such assumptions exist all over > > >net/sched? > > > > Not only net/sched. The C standard requires that free(NULL) works. > > Thanks for clarifying this. > > cheers, > jamal > > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak in net/sched/ipt.c?
On Wed, 2005-03-23 at 08:40, Jan Engelhardt wrote: > >I have seen people put little comments of "kfree will work if you > >pass it NULL" - are you saying such assumptions exist all over > >net/sched? > > Not only net/sched. The C standard requires that free(NULL) works. Thanks for clarifying this. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak in net/sched/ipt.c?
On Wed, 2005-03-23 at 08:23, Thomas Graf wrote: > > kfree simply does nothing if it is given a null pointer so that > goto rtattr_failure for t == NULL is handled just fine without > a check. I will never get used to this behaviour and policy as > well though, it somewhat makes code less readable. > I cant get used to it either; actually i dont think i would be able to stop my fingers ;-> > > didnt understand the janitor part. > > It will probably be removed again by one of the regular 'remove > unnecessary pre kfree checks' patchsets. > Yes, already Jan Engelhardt <[EMAIL PROTECTED]> has made that point - although he did not post to netdev!! ;-> So ignore my comment Herbert cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak in net/sched/ipt.c?
On Wed, 2005-03-23 at 07:55, Thomas Graf wrote: > * jamal <[EMAIL PROTECTED]> 2005-03-23 07:40 > > Just a small correction to patchlet: > > The second kfree should check for existence of t. > > t is either valid or NULL so it's not a problem, unless you want > to create janitor work of course. ;-> if t is null you still goto rtattr_failure I have seen people put little comments of "kfree will work if you pass it NULL" - are you saying such assumptions exist all over net/sched? didnt understand the janitor part. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak in net/sched/ipt.c?
On Wed, 2005-03-23 at 06:30, Herbert Xu wrote: > Yichen Xie <[EMAIL PROTECTED]> wrote: > > Is the memory block allocated on line 315 leaked every time tcp_ipt_dump > > is called? > > It seems to be. This patch should free it. > > Signed-off-by: Herbert Xu <[EMAIL PROTECTED]> > > BTW, please report networking bugs to [EMAIL PROTECTED] > Thanks for pointing this out - I _know_ its in the kernel list FAQ. I was sitting beside R Gooch when he added "thou shalt post to netdev". And of course i am gonna bitch about it every time someone doesnt post to netdev;-> Just a small correction to patchlet: The second kfree should check for existence of t. cheers, jamal > Thanks, - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] updated, ethernet-bridge: update skb->priority in case forwarded frame has VLAN-header
Actually there is a case to be made for this to be part of the bridge code. A VLAN is a single collision domain which is mappable to a collission domain that is a bridge. infact the old VLAN code written by Lennert (and somebody else) had those two intermingled. cheers, jamal On Fri, 2005-03-11 at 12:37, Stephen Hemminger wrote: > On Fri, 11 Mar 2005 17:20:22 +0300 > Leo Yuriev <[EMAIL PROTECTED]> wrote: > > > Kernel 2.6 (2.6.11) > > > > When ethernet-bridge forward a packet and such ethernet-frame has > > VLAN-tag, bridge should update skb->prioriry for properly QoS > > handling. This small patch does this. > > > > Based upon discussion during last week I added pskb_may_pull() > > checking and simple mapping from 802.1p/user_priority to skb->priority. > > > > Patch-by: Leo Yuriev <[EMAIL PROTECTED]> > > Do this as an ebtables module please. > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Lse-tech] Re: A common layer for Accounting packages
On Mon, 2005-02-28 at 09:25, Thomas Graf wrote: > * jamal <[EMAIL PROTECTED]> 2005-02-28 09:10 [..] > > To justify writting the new code, I am assuming someone has actually sat > > down and in the minimal stuck their finger in the air > > and said "yes, there is definetely wind there". > > I did, not for this problem though. The code this idea comes from sends > batched events I would bet the benefit you are seeing has to do with batching rather than such an optimization flag. Different ballgame. I relooked at their code snippet, they dont even have skbs built nor even figured out what sock or PID. That work still needs to be done it seems in cn_netlink_send(). So probably all they need to do is move the check in cn_netlink_send() instead. This is assuming they are not scratching their heads with some realted complexities. I am gonna disapear for a while; hopefully the original posters have gathered some ideas from what we discussed. cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Lse-tech] Re: A common layer for Accounting packages
On Mon, 2005-02-28 at 08:53, Thomas Graf wrote: > * jamal <[EMAIL PROTECTED]> 2005-02-28 08:40 > > > > netlink broadcast or a wrapper around it. > > Why even bother doing the check with netlink_has_listeners()? > > To implement the master enable/disable switch they want. The messages > don't get send out anyway but why bother doing all the work if nothing > will get send out in the end? It implements a well defined flag > controlled by open/close on fds (thus handles dying applications) > stating whether the whole code should be enabled or disabled. It is of > course not needed to avoid sending unnecessary messages. To justify writting the new code, I am assuming someone has actually sat down and in the minimal stuck their finger in the air and said "yes, there is definetely wind there". Which leadsto Marcello's question in other email: Theres some overhead. - Message needs to be built with skbs allocated (not the cn_xxx thing that seems to be invoked - I suspect that thing will build the skbs); - the netlink table needs to be locked -and searched and only then do you find theres nothing to send to. The point i was making is if you actually had to post this question, then you must be running into some problems of complexity ;-> which implies to me that the delta overhead maybe worth it compared to introducing the complexity or any new code. I wasnt involved in the discussion - I just woke up and saw the posting and was bored. So the justification for the optimization has probably been explained and it may be worth doing the check (but probably such check should go into whatever that cn_xxx is). cheers, jamal - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Lse-tech] Re: A common layer for Accounting packages
netlink broadcast or a wrapper around it.
Why even bother doing the check with netlink_has_listeners()?
cheers,
jamal
On Mon, 2005-02-28 at 08:20, Thomas Graf wrote:
> > Havent seen the beginnings of this thread. But whatever you are trying
> > to do seems to suggest some complexity that you are trying to
> > workaround. What was wrong with just going ahead and just always
> > invoking your netlink_send()?
>
> I guess parts of the wheel are broken and need to be reinvented ;->
>
> > If there are nobody in user space (or kernel) listening, it wont go
> > anywhere.
>
> Additional you may want to extend netlink a bit to check whether
> there is a listener before creating the messages. The method to do so
> depends on whether you use netlink_send() or netlink_brodacast(). The
> latter is more flexiable because you can add more groups later on
> and the userspace applications can decicde which ones they want to
> listen to. Both methods handle dying clients perfectly fine, the
> association to the netlink socket gets destroyed as soon as the socket
> is closed. Therefore you can simply check mc_list of the netlink
> protocol you use to see if there are any listeners registered:
>
> static inline int netlink_has_listeners(struct sock *sk)
> {
> int ret;
>
> read_lock(&nl_table_lock);
> ret = list_empty(&nl_table[sk->sk_protocol].mc_list)
> read_unlock(&nl_table_lock);
>
> return !ret;
> }
>
> This is simplified and ignores the actual group assignments, i.e. you
> might want to extend it to have it check if there are listeners for
> a certain group.
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
