* Re: [RFC PATCH v3 0/6] Configuring traffic classes via new hardware offload mechanism in tc/mqprio
From: Florian Fainelli @ 2017-09-07 18:34 UTC (permalink / raw)
To: Amritha Nambiar, intel-wired-lan, jeffrey.t.kirsher
Cc: alexander.h.duyck, netdev
In-Reply-To: <150478158684.24662.17975701233699487888.stgit@anamdev.jf.intel.com>
On 09/07/2017 04:00 AM, Amritha Nambiar wrote:
> The following series introduces a new hardware offload mode in
> tc/mqprio where the TCs, the queue configurations and
> bandwidth rate limits are offloaded to the hardware. The existing
> mqprio framework is extended to configure the queue counts and
> layout and also added support for rate limiting. This is achieved
> through new netlink attributes for the 'mode' option which takes
> values such as 'dcb' (default) and 'channel' and a 'shaper' option
> for QoS attributes such as bandwidth rate limits in hw mode 1.
So "dcb" defines a default priorities to queue mapping?
> Legacy devices can fall back to the existing setup supporting hw mode
> 1 without these additional options where only the TCs are offloaded
> and then the 'mode' and 'shaper' options defaults to DCB support.
That's the last part that confuses me, see below.
> The i40e driver enables the new mqprio hardware offload mechanism
> factoring the TCs, queue configuration and bandwidth rates by
> creating HW channel VSIs.
I am really confused by what you call hw_mode 1, as I understand it
there are really 3 different modes:
- legacy: you don't define any traffic class mapping, but you can still
chain this scheduler with a match + action (like what
Documentation/networking/multiqueue.txt) you can optionally also add
"shaper" arguments, but there should not be any default DCB queue
mapping either?
- dcb: a default mapping for traffic classes to queues is defined,
optional "shaper" arguments
- channel: (maybe calling that "custom_tc_map" would be clearer?) where
you express the exact traffic classes to queue mapping and optional
"shaper" arguments
I think that's what you are doing, but I just got confused by the cover
letter.
>
> In this new mode, the priority to traffic class mapping and the
> user specified queue ranges are used to configure the traffic
> class when the 'mode' option is set to 'channel'. This is achieved by
> creating HW channels(VSI). A new channel is created for each of the
> traffic class configuration offloaded via mqprio framework except for
> the first TC (TC0) which is for the main VSI. TC0 for the main VSI is
> also reconfigured as per user provided queue parameters. Finally,
> bandwidth rate limits are set on these traffic classes through the
> shaper attribute by sending these rates in addition to the number of
> TCs and the queue configurations.
>
> Example:
> # tc qdisc add dev eth0 root mqprio num_tc 2 map 0 0 0 0 1 1 1 1\
> queues 4@0 4@4 hw 1 mode channel shaper bw_rlimit\
Do you see a case where you can declare a different number of traffic
classes say 4 and map them onto just 2 hardware queues? If not, it seems
a tiny bit redundant to have to specify both the map and the queue
mapping should be sufficient, right?
> min_rate 1Gbit 2Gbit max_rate 4Gbit 5Gbit
>
> To dump the bandwidth rates:
>
> # tc qdisc show dev eth0
>
> qdisc mqprio 804a: root tc 2 map 0 0 0 0 1 1 1 1 0 0 0 0 0 0 0 0
> queues:(0:3) (4:7)
> mode:channel
> shaper:bw_rlimit min_rate:1Gbit 2Gbit max_rate:4Gbit 5Gbit
I am not well versed into tc, but being able to specify "shaper"
arguments has actually value outside of just the multiq scheduler and it
could probably be an action on its own?
>
> ---
>
> Amritha Nambiar (6):
> mqprio: Introduce new hardware offload mode and shaper in mqprio
> i40e: Add macro for PF reset bit
> i40e: Add infrastructure for queue channel support
> i40e: Enable 'channel' mode in mqprio for TC configs
> i40e: Refactor VF BW rate limiting
> i40e: Add support setting TC max bandwidth rates
>
>
> drivers/net/ethernet/intel/i40e/i40e.h | 44 +
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 3
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 8
> drivers/net/ethernet/intel/i40e/i40e_main.c | 1463 +++++++++++++++++---
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 2
> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 50 -
> include/net/pkt_cls.h | 9
> include/uapi/linux/pkt_sched.h | 32
> net/sched/sch_mqprio.c | 183 ++-
> 9 files changed, 1551 insertions(+), 243 deletions(-)
>
> --
>
--
Florian
^ permalink raw reply
* Re: [Patch net v2 2/2] net_sched: fix all the madness of tc filter chain
From: Cong Wang @ 2017-09-07 17:45 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jakub Kicinski, Jiri Pirko
In-Reply-To: <20170907063216.GB1967@nanopsycho>
On Wed, Sep 6, 2017 at 11:32 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Sep 07, 2017 at 06:26:07AM CEST, xiyou.wangcong@gmail.com wrote:
>>This patch fixes the following madness of tc filter chain:
>
> Could you avoid expressive words like "madness" and such?
> Please be technical.
>
If the following 2a) 2b) 2c) 2d) are not enough to show the madness,
I don't know any other to show it. Madness is for code, not for you
or any other person, so 100% technical.
>
>>
>>1) tcf_chain_destroy() is called by both tcf_block_put() and
>> tcf_chain_put(). tcf_chain_put() is correctly refcnt'ed and paired
>> with tcf_chain_get(), but tcf_block_put() is not, it should be paired
>> with tcf_block_get() which means we still need to decrease the refcnt.
>> Think it in another way: if we call tcf_bock_put() immediately after
>> tcf_block_get(), could we get effectively a nop? This causes a memory
>> leak as reported by Jakub.
>>
>>2) tp proto should hold a refcnt to the chain too. This significantly
>> simplifies the logic:
>>
>>2a) Chain 0 is no longer special, it is created and refcnted by tp
>> like any other chains. All the ugliness in tcf_chain_put() can be
>> gone!
>>
>>2b) No need to handle the flushing oddly, because block still holds
>> chain 0, it can not be released, this guarantees block is the last
>> user.
>>
>>2c) The race condition with RCU callbacks is easier to handle with just
>> a rcu_barrier()! Much easier to understand, nothing to hide! Thanks
>> to the previous patch. Please see also the comments in code.
>>
>>2d) Make the code understandable by humans, much less error-prone.
>>
>>Fixes: 744a4cf63e52 ("net: sched: fix use after free when tcf_chain_destroy is called multiple times")
>>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>Cc: Jiri Pirko <jiri@mellanox.com>
>>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>---
>> net/sched/cls_api.c | 38 ++++++++++++++++++++++----------------
>> 1 file changed, 22 insertions(+), 16 deletions(-)
>>
>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>index 6c5ea84d2682..e9060dc36519 100644
>>--- a/net/sched/cls_api.c
>>+++ b/net/sched/cls_api.c
>>@@ -209,21 +209,20 @@ static void tcf_chain_flush(struct tcf_chain *chain)
>> RCU_INIT_POINTER(*chain->p_filter_chain, NULL);
>> while ((tp = rtnl_dereference(chain->filter_chain)) != NULL) {
>> RCU_INIT_POINTER(chain->filter_chain, tp->next);
>>+ tcf_chain_put(chain);
>> tcf_proto_destroy(tp);
>> }
>> }
>>
>> static void tcf_chain_destroy(struct tcf_chain *chain)
>> {
>>- /* May be already removed from the list by the previous call. */
>>- if (!list_empty(&chain->list))
>>- list_del_init(&chain->list);
>>+ list_del(&chain->list);
>>+ kfree(chain);
>>+}
>>
>>- /* There might still be a reference held when we got here from
>>- * tcf_block_put. Wait for the user to drop reference before free.
>>- */
>>- if (!chain->refcnt)
>>- kfree(chain);
>>+static void tcf_chain_hold(struct tcf_chain *chain)
>>+{
>>+ ++chain->refcnt;
>> }
>>
>> struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>@@ -233,7 +232,7 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>
>> list_for_each_entry(chain, &block->chain_list, list) {
>> if (chain->index == chain_index) {
>>- chain->refcnt++;
>>+ tcf_chain_hold(chain);
>> return chain;
>> }
>> }
>>@@ -246,10 +245,7 @@ EXPORT_SYMBOL(tcf_chain_get);
>>
>> void tcf_chain_put(struct tcf_chain *chain)
>> {
>>- /* Destroy unused chain, with exception of chain 0, which is the
>>- * default one and has to be always present.
>>- */
>>- if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
>>+ if (--chain->refcnt == 0)
>
> Okay, so you take the reference for every goto_chain action and every
> tp, right? Note that for chain 0, you hold one more reference (due to
> the creation). That is probably ok as we need chain 0 not to go away
> even if all tps and goto_chain actions are gone.
Yeah, this is the core of the patch.
>
>
>> tcf_chain_destroy(chain);
>> }
>> EXPORT_SYMBOL(tcf_chain_put);
>>@@ -294,10 +290,18 @@ void tcf_block_put(struct tcf_block *block)
>> if (!block)
>> return;
>>
>>- list_for_each_entry_safe(chain, tmp, &block->chain_list, list) {
>>+ /* Standalone actions are not allowed to jump to any chain, and
>>+ * bound actions should be all removed after flushing. However,
>>+ * filters are destroyed in RCU callbacks, we have to flush and wait
>>+ * for them before releasing this refcnt, otherwise we race with RCU
>>+ * callbacks!!!
>
> Why the "!!!"? Please avoid that. Not necessary at all.
Because we don't have lock here, we have to pay more extra
attention to it. Playing list without lock is like playing fire. I use "!!!" to
draw people's attention when they touch it, perhaps "XXX" is better?
>
>
>>+ */
>>+ list_for_each_entry(chain, &block->chain_list, list)
>> tcf_chain_flush(chain);
>>- tcf_chain_destroy(chain);
>>- }
>>+ rcu_barrier();
>
> This actually tries to fix another bug I discovered yesterday. Good.
>
This on the other hand proves we should make the code clean
and understandable asap, not just wait for net-next.
>
>>+
>>+ list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
>>+ tcf_chain_put(chain);
>
> Which reference are you putting here? For chain 0, that is the original
> reference due to creation from block_get. But how about the other
> chains? If you do flush all in the previous list iteration, they are
> removed there. Also note that they are removed from the list while
> iterating it.
Yes it is for chain 0, because block holds a reference to chain 0 during
creation. Non-0 chains are created with refcnt==1 too but paired with
tcf_chain_put() rather than tcf_block_put(). This is what makes chain 0
not special w.r.t. refcnt.
>
> I believe that you need to add tcf_chain_hold(chain) to the start of the
> previous list iteration to ensure all existing chains will stay, then
> you can put them here.
We don't need it, it is already perfectly paired with tcf_block_get().
Think about the following:
1)
tcf_block_get(...); // create chain 0 with refcnt=1
tcf_block_put(); // no tp, only chain 0 in chain_list
// chain 0 is put, refcnt=0, it is gone
2)
tcf_block_get(); // create chain 0 with refcnt=1
...
tcf_chain_get(11); // create chain 11 with refcnt=1
// one tp is inserted to chain 11, now refcnt==2
// in tc_ctl_tfilter()
tcf_chain_put(11); // paired with above get, refcnt==1
...
tcf_block_put(); // flush chain 11, tp removed, refcnt==0
// put chain 0 too, paired with block get, refcnt == 0
// both chain 0 and chain11 are gone
>
> Did you test this? I believe we need some simple test script.
Of course I did. I verified memleak is gone and tested basic
filters and gact actions with chain 0 and chain 11, everything
works as expected, I also added a printk() to verify the chain
is really gone as soon as all references are gone.
I will contribute my test cases back after I figure out how.
Also net-next is already closed.
^ permalink raw reply
* Re: [Intel-wired-lan] [RFC PATCH v3 0/6] Configuring traffic classes via new hardware offload mechanism in tc/mqprio
From: Shannon Nelson @ 2017-09-07 17:38 UTC (permalink / raw)
To: Nambiar, Amritha, intel-wired-lan, jeffrey.t.kirsher; +Cc: netdev
In-Reply-To: <bd18cda5-4827-2246-f736-630527a9fcbc@intel.com>
On 9/7/2017 10:22 AM, Nambiar, Amritha wrote:
> On 9/7/2017 9:45 AM, Shannon Nelson wrote:
[...]
>>
>> It would be nice to know what has changed since the last review, either
>> summarized here or in the individual patch files.
[...]
>
> For all those patch files that have changed since the last revision, I
> have captured all the new changes in the section titled "v3: " of each
> patch file.
Mea culpa.
ugh.
sln
^ permalink raw reply
* Re: [Intel-wired-lan] [RFC PATCH v3 0/6] Configuring traffic classes via new hardware offload mechanism in tc/mqprio
From: Nambiar, Amritha @ 2017-09-07 17:22 UTC (permalink / raw)
To: Shannon Nelson, intel-wired-lan, jeffrey.t.kirsher; +Cc: netdev
In-Reply-To: <a37ba2ca-145a-0228-961d-c805434e7d40@oracle.com>
On 9/7/2017 9:45 AM, Shannon Nelson wrote:
> On 9/7/2017 4:00 AM, Amritha Nambiar wrote:
>> The following series introduces a new hardware offload mode in
>> tc/mqprio where the TCs, the queue configurations and
>> bandwidth rate limits are offloaded to the hardware. The existing
>> mqprio framework is extended to configure the queue counts and
>> layout and also added support for rate limiting. This is achieved
>> through new netlink attributes for the 'mode' option which takes
>> values such as 'dcb' (default) and 'channel' and a 'shaper' option
>> for QoS attributes such as bandwidth rate limits in hw mode 1.
>> Legacy devices can fall back to the existing setup supporting hw mode
>> 1 without these additional options where only the TCs are offloaded
>> and then the 'mode' and 'shaper' options defaults to DCB support.
>> The i40e driver enables the new mqprio hardware offload mechanism
>> factoring the TCs, queue configuration and bandwidth rates by
>> creating HW channel VSIs.
>>
>> In this new mode, the priority to traffic class mapping and the
>> user specified queue ranges are used to configure the traffic
>> class when the 'mode' option is set to 'channel'. This is achieved by
>> creating HW channels(VSI). A new channel is created for each of the
>> traffic class configuration offloaded via mqprio framework except for
>> the first TC (TC0) which is for the main VSI. TC0 for the main VSI is
>> also reconfigured as per user provided queue parameters. Finally,
>> bandwidth rate limits are set on these traffic classes through the
>> shaper attribute by sending these rates in addition to the number of
>> TCs and the queue configurations.
>>
>> Example:
>> # tc qdisc add dev eth0 root mqprio num_tc 2 map 0 0 0 0 1 1 1 1\
>> queues 4@0 4@4 hw 1 mode channel shaper bw_rlimit\
>> min_rate 1Gbit 2Gbit max_rate 4Gbit 5Gbit
>>
>> To dump the bandwidth rates:
>>
>> # tc qdisc show dev eth0
>>
>> qdisc mqprio 804a: root tc 2 map 0 0 0 0 1 1 1 1 0 0 0 0 0 0 0 0
>> queues:(0:3) (4:7)
>> mode:channel
>> shaper:bw_rlimit min_rate:1Gbit 2Gbit max_rate:4Gbit 5Gbit
>>
>> ---
>>
>> Amritha Nambiar (6):
>> mqprio: Introduce new hardware offload mode and shaper in mqprio
>> i40e: Add macro for PF reset bit
>> i40e: Add infrastructure for queue channel support
>> i40e: Enable 'channel' mode in mqprio for TC configs
>> i40e: Refactor VF BW rate limiting
>> i40e: Add support setting TC max bandwidth rates
>>
>
> It would be nice to know what has changed since the last review, either
> summarized here or in the individual patch files. This helps in knowing
> how much attention should be given to this new set of patches, and
> encourages further review. I don't remember seeing any responses to my
> previous comments, and it looks like not all of them were acted upon.
For all those patch files that have changed since the last revision, I
have captured all the new changes in the section titled "v3: " of each
patch file. Also, I had replied to most of your comments that they'll be
fixed in v3 and the one in the mqprio patch that error handling was
already being done and these responses can be verified in patchwork. I
missed to respond to your comment regarding supporting macvlan offloads
through these channels, that will not be a part of this series and will
be looked upon at a later time in a subsequent patch series.
>
> sln
>
>>
>> drivers/net/ethernet/intel/i40e/i40e.h | 44 +
>> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 3
>> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 8
>> drivers/net/ethernet/intel/i40e/i40e_main.c | 1463 +++++++++++++++++---
>> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 2
>> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 50 -
>> include/net/pkt_cls.h | 9
>> include/uapi/linux/pkt_sched.h | 32
>> net/sched/sch_mqprio.c | 183 ++-
>> 9 files changed, 1551 insertions(+), 243 deletions(-)
>>
>> --
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>>
^ permalink raw reply
* Re: Bad escapes in ip -online
From: Stephen Hemminger @ 2017-09-07 16:56 UTC (permalink / raw)
To: John Kodis; +Cc: netdev
In-Reply-To: <b4972c84-c827-3a05-2267-bbcadf101260@nasa.gov>
On Thu, 7 Sep 2017 12:08:56 -0400
John Kodis <john.m.kodis@nasa.gov> wrote:
> The -online option to the 'ip link', 'ip addr', and perhaps others is
> putting out a backslash in places where a newline character should go,
> as so:
>
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode
> DEFAULT group default qlen 1000\ link/loopback 00:00:00:00:00:00 brd
> 00:00:00:00:00:00
>
> -- John.
>
The backslash is the virtual line separator in the one line format.
It is has always been that way, and programs parse the oneline output.
^ permalink raw reply
* Re: nfp bpf offload add/replace
From: Jakub Kicinski @ 2017-09-07 16:52 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, mlxsw, Daniel Borkmann, Simon Horman
In-Reply-To: <20170907140503.GE1967@nanopsycho>
On Thu, 7 Sep 2017 16:05:03 +0200, Jiri Pirko wrote:
> Thu, Sep 07, 2017 at 03:44:12PM CEST, kubakici@wp.pl wrote:
> >On Thu, 7 Sep 2017 11:10:33 +0200, Jiri Pirko wrote:
> >> Hi Kuba.
> >>
> >> I'm looking into cls_bpf code and nfp_net_bpf_offload function in your
> >> driver. Why do you need TC_CLSBPF_ADD? Seems like TC_CLSBPF_REPLACE
> >> should be enough. It would make the cls_bpf code easier.
> >>
> >> Note that other cls just have replace/destroy (u32 too, as drivers
> >> handle NEW/REPLACE in one switch-case - will patch this).
> >
> >Could we clarify what the REPLACE is actually supposed to do? :)
> >
> >In the flower code and the REPLACE looks a lot like ADD on the
> >surface... If change is called it will invoke REPLACE with the new
> >filter and then if there was an old filter, it will do DELETE. Is my
> >understanding correct?
>
> Yes, correct.
>
> >
> >If so I found this model of operation somehow confusing. Plus the
> >management of flows may get slightly tricky if there is a possibility of
> >"replacing" a flow with an identical one. Flower may make calls like
> >these:
> >
> >add flower vlan_id 100 action ...
> ># REPLACE vid 100 ...
> >change ... flower vlan_id 100 action ...
> ># REPLACE vid 100 ...
> ># DELETE vid 100 ...
>
> Yes, that is the flow.
>
> >
> >Doesn't this force driver/HW to implement refcounting on the rules?
>
> Why do you think so? There is a cookie that is passed from flower down
> and driver uses it to remove the entry.
Right, the key/mask combination doesn't have to be unique anyway...
> >On why I need the replace - BPF unlike other classifiers usually
> >installs a single program, I think offloading multiple TC filters is
> >questionable (people will use tailcalls instead most likely). I want to
> >be able to implement atomic replace of that single program (i.e. not ADD
> >followed by DELETE) because that simplifies the driver quite a bit.
>
> Understood. So, looks like the REPLACE/DESTROY would be sufficient for
> bpf. ADD is not needed as it can be done by REPLACE-NULL, right?
Yes, or you could take it to the extreme ;)
DESTROY == offload(old, NULL)
ADD == offload(NULL, new)
REPLACE == offload(obj, new)
> On the other hand, the rest of the cls, namely flower, u32 and matchall
> need ADD/DESTROY as they don't really do no replacing.
>
> Makes sense?
Ack, if you're unifying things, I don't mind how things are muxed as
long as atomic replace is possible.
FWIW cls_bpf doesn't pass old prog in REPLACE right now, but I have
patch to add it anyway since it simplifies the driver when maps are
involved. I should probably stop looking at the .command completely,
just rely on new/old programs being populated.
^ permalink raw reply
* Re: [Intel-wired-lan] [RFC PATCH v3 0/6] Configuring traffic classes via new hardware offload mechanism in tc/mqprio
From: Shannon Nelson @ 2017-09-07 16:45 UTC (permalink / raw)
To: Amritha Nambiar, intel-wired-lan, jeffrey.t.kirsher; +Cc: netdev
In-Reply-To: <150478158684.24662.17975701233699487888.stgit@anamdev.jf.intel.com>
On 9/7/2017 4:00 AM, Amritha Nambiar wrote:
> The following series introduces a new hardware offload mode in
> tc/mqprio where the TCs, the queue configurations and
> bandwidth rate limits are offloaded to the hardware. The existing
> mqprio framework is extended to configure the queue counts and
> layout and also added support for rate limiting. This is achieved
> through new netlink attributes for the 'mode' option which takes
> values such as 'dcb' (default) and 'channel' and a 'shaper' option
> for QoS attributes such as bandwidth rate limits in hw mode 1.
> Legacy devices can fall back to the existing setup supporting hw mode
> 1 without these additional options where only the TCs are offloaded
> and then the 'mode' and 'shaper' options defaults to DCB support.
> The i40e driver enables the new mqprio hardware offload mechanism
> factoring the TCs, queue configuration and bandwidth rates by
> creating HW channel VSIs.
>
> In this new mode, the priority to traffic class mapping and the
> user specified queue ranges are used to configure the traffic
> class when the 'mode' option is set to 'channel'. This is achieved by
> creating HW channels(VSI). A new channel is created for each of the
> traffic class configuration offloaded via mqprio framework except for
> the first TC (TC0) which is for the main VSI. TC0 for the main VSI is
> also reconfigured as per user provided queue parameters. Finally,
> bandwidth rate limits are set on these traffic classes through the
> shaper attribute by sending these rates in addition to the number of
> TCs and the queue configurations.
>
> Example:
> # tc qdisc add dev eth0 root mqprio num_tc 2 map 0 0 0 0 1 1 1 1\
> queues 4@0 4@4 hw 1 mode channel shaper bw_rlimit\
> min_rate 1Gbit 2Gbit max_rate 4Gbit 5Gbit
>
> To dump the bandwidth rates:
>
> # tc qdisc show dev eth0
>
> qdisc mqprio 804a: root tc 2 map 0 0 0 0 1 1 1 1 0 0 0 0 0 0 0 0
> queues:(0:3) (4:7)
> mode:channel
> shaper:bw_rlimit min_rate:1Gbit 2Gbit max_rate:4Gbit 5Gbit
>
> ---
>
> Amritha Nambiar (6):
> mqprio: Introduce new hardware offload mode and shaper in mqprio
> i40e: Add macro for PF reset bit
> i40e: Add infrastructure for queue channel support
> i40e: Enable 'channel' mode in mqprio for TC configs
> i40e: Refactor VF BW rate limiting
> i40e: Add support setting TC max bandwidth rates
>
It would be nice to know what has changed since the last review, either
summarized here or in the individual patch files. This helps in knowing
how much attention should be given to this new set of patches, and
encourages further review. I don't remember seeing any responses to my
previous comments, and it looks like not all of them were acted upon.
sln
>
> drivers/net/ethernet/intel/i40e/i40e.h | 44 +
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 3
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 8
> drivers/net/ethernet/intel/i40e/i40e_main.c | 1463 +++++++++++++++++---
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 2
> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 50 -
> include/net/pkt_cls.h | 9
> include/uapi/linux/pkt_sched.h | 32
> net/sched/sch_mqprio.c | 183 ++-
> 9 files changed, 1551 insertions(+), 243 deletions(-)
>
> --
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>
^ permalink raw reply
* Re: pull-request: mac80211 2017-09-07
From: David Miller @ 2017-09-07 16:41 UTC (permalink / raw)
To: johannes; +Cc: netdev, linux-wireless
In-Reply-To: <20170907070939.3658-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 7 Sep 2017 09:09:38 +0200
> During my long absence some things have accumulated, but there wasn't
> actually all that much that could've gone into the last cycle, and a
> fix or two was taken care of by others.
>
> The most important thing here is probably the deadlock fix that a few
> people have run into on 4.13, but that was only identified now, and
> perhaps the 40 MHz fix from Emmanuel that helps avoid iwlwifi firmware
> crashes.
>
> Please pull and let me know if there's any problem.
Pulled, thanks.
^ permalink raw reply
* Re: iwlwifi: mvm: only send LEDS_CMD when the FW supports it
From: Kalle Valo @ 2017-09-07 16:40 UTC (permalink / raw)
To: Luciano Coelho
Cc: torvalds, linux-wireless, johannes, linux-kernel, akpm, netdev,
davem, emmanuel.grumbach, Luca Coelho
In-Reply-To: <20170907075152.4522-1-luca@coelho.fi>
Luciano Coelho <luca@coelho.fi> wrote:
> From: Luca Coelho <luciano.coelho@intel.com>
>
> The LEDS_CMD command is only supported in some newer FW versions
> (e.g. iwlwifi-8000C-31.ucode), so we can't send it to older versions
> (such as iwlwifi-8000C-27.ucode).
>
> To fix this, check for a new bit in the FW capabilities TLV that tells
> when the command is supported.
>
> Note that the current version of -31.ucode in linux-firmware.git
> (31.532993.0) does not have this capability bit set, so the LED won't
> work, even though this version should support it. But we will update
> this firmware soon, so it won't be a problem anymore.
>
> Fixes: 7089ae634c50 ("iwlwifi: mvm: use firmware LED command where applicable")
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
Patch applied to wireless-drivers.git, thanks.
2eabc84d2f8e iwlwifi: mvm: only send LEDS_CMD when the FW supports it
--
https://patchwork.kernel.org/patch/9941719/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] iwlwifi: mvm: only send LEDS_CMD when the FW supports it
From: Kalle Valo @ 2017-09-07 16:36 UTC (permalink / raw)
To: Linus Torvalds
Cc: Luca Coelho, Linux Wireless List, Johannes Berg,
Linux Kernel Mailing List, Andrew Morton, Network Development,
David Miller, Emmanuel Grumbach, Luca Coelho
In-Reply-To: <CA+55aFyeEwg7vd8FPHqG+4d7KbJ6ke4NDwhd+8y=_auRApyvmw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Thu, Sep 7, 2017 at 5:39 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Linus, do you want to apply this directly or should we take it via the
>> normal route (wireless-drivers -> net)? If your prefer the latter when
>> I'm planning to submit this to Dave in a day or two and expecting it to
>> get to your tree in about a week, depending of course what is Dave's
>> schedule.
>
> Since we have a workaround for the problem, let's just go through the
> regular channels. As long as I get the fix through David before the
> merge window closes, I'm happy.
Ok, I'll aim to send the pull request to Dave tomorrow.
--
Kalle Valo
^ permalink raw reply
* Re: [RFC net-next 0/5] TSN: Add qdisc-based config interfaces for traffic shapers
From: Henrik Austad @ 2017-09-07 16:18 UTC (permalink / raw)
To: Richard Cochran
Cc: Vinicius Costa Gomes, netdev, jhs, xiyou.wangcong, jiri,
intel-wired-lan, andre.guedes, ivan.briano,
jesus.sanchez-palencia, boon.leong.ong
In-Reply-To: <20170907155315.5gqy5e4susl25wa2@localhost>
[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]
On Thu, Sep 07, 2017 at 05:53:15PM +0200, Richard Cochran wrote:
> On Thu, Sep 07, 2017 at 05:27:51PM +0200, Henrik Austad wrote:
> > On Thu, Sep 07, 2017 at 02:40:18PM +0200, Richard Cochran wrote:
> > And if you want to this driver to act as a bridge, how do you accomodate
> > change in network requirements? (i.e. how does this work with switchdev?)
>
> To my understanding, this Qdisc idea provides QoS for the host's
> transmitted traffic, and nothing more.
Ok, then we're on the same page.
> > - Or am I overthinking this?
>
> Being able to configure the external ports of a switchdev is probably
> a nice feature, but that is another story. (But maybe I misunderstood
> the authors' intent!)
ok, chalk that one up for later perhaps
> > If you have more than 1 application in userspace that wants to send data
> > using this scheduler, how do you ensure fair transmission of frames? (both
> > how much bandwidth they use,
>
> There are many ways to handle this, and we shouldn't put any of that
> policy into the kernel. For example, there might be a monolithic
> application with configurable threads, or an allocation server that
> grants bandwidth to applications via IPC, or a multiplexing stream
> server like jack, pulse, etc, and so on...
true
> > but also ordering of frames from each application)
>
> Not sure what you mean by this.
Fair enough, I'm not that good at making myself clear :)
Let's see if I can make a better attempt:
If you have 2 separate applications that have their own streams going to
different endpoints - but both are in the same class, then they will
share the qdisc bandwidth.
So application
- A sends frame A1, A2, A3, .. An
- B sends B1, B2, .. Bn
What I was trying to describe was: if application A send 2 frames, and B
sends 2 frames at the same time, then you would hope that the order would
be A1, B1, A2, B2, and not A1, A2, B1, B2.
None of this would be a problem if you expect a *single* user, like the
allocation server you described above. Again, I think this is just me
overthinking the problem right now :)
> > Do you expect all of this to be handled in userspace?
>
> Yes, I do.
ok, fair enough
Thanks for answering my questions!
--
Henrik Austad
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Bad escapes in ip -online
From: John Kodis @ 2017-09-07 16:08 UTC (permalink / raw)
To: netdev
The -online option to the 'ip link', 'ip addr', and perhaps others is
putting out a backslash in places where a newline character should go,
as so:
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode
DEFAULT group default qlen 1000\ link/loopback 00:00:00:00:00:00 brd
00:00:00:00:00:00
-- John.
^ permalink raw reply
* Re: [PATCH] iwlwifi: mvm: only send LEDS_CMD when the FW supports it
From: Linus Torvalds @ 2017-09-07 16:13 UTC (permalink / raw)
To: Kalle Valo
Cc: Luca Coelho, Linux Wireless List, Johannes Berg,
Linux Kernel Mailing List, Andrew Morton, Network Development,
David Miller, Emmanuel Grumbach, Luca Coelho
In-Reply-To: <877exan0nq.fsf-HodKDYzPHsUD5k0oWYwrnHL1okKdlPRT@public.gmane.org>
On Thu, Sep 7, 2017 at 5:39 AM, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
> Linus, do you want to apply this directly or should we take it via the
> normal route (wireless-drivers -> net)? If your prefer the latter when
> I'm planning to submit this to Dave in a day or two and expecting it to
> get to your tree in about a week, depending of course what is Dave's
> schedule.
Since we have a workaround for the problem, let's just go through the
regular channels. As long as I get the fix through David before the
merge window closes, I'm happy.
Linus
^ permalink raw reply
* Re: [RFC net-next 0/5] TSN: Add qdisc-based config interfaces for traffic shapers
From: Richard Cochran @ 2017-09-07 15:53 UTC (permalink / raw)
To: Henrik Austad
Cc: Vinicius Costa Gomes, netdev, jhs, xiyou.wangcong, jiri,
intel-wired-lan, andre.guedes, ivan.briano,
jesus.sanchez-palencia, boon.leong.ong
In-Reply-To: <20170907152751.GA9064@sisyphus.home.austad.us>
On Thu, Sep 07, 2017 at 05:27:51PM +0200, Henrik Austad wrote:
> On Thu, Sep 07, 2017 at 02:40:18PM +0200, Richard Cochran wrote:
> And if you want to this driver to act as a bridge, how do you accomodate
> change in network requirements? (i.e. how does this work with switchdev?)
To my understanding, this Qdisc idea provides QoS for the host's
transmitted traffic, and nothing more.
> - Or am I overthinking this?
Being able to configure the external ports of a switchdev is probably
a nice feature, but that is another story. (But maybe I misunderstood
the authors' intent!)
> If you have more than 1 application in userspace that wants to send data
> using this scheduler, how do you ensure fair transmission of frames? (both
> how much bandwidth they use,
There are many ways to handle this, and we shouldn't put any of that
policy into the kernel. For example, there might be a monolithic
application with configurable threads, or an allocation server that
grants bandwidth to applications via IPC, or a multiplexing stream
server like jack, pulse, etc, and so on...
> but also ordering of frames from each application)
Not sure what you mean by this.
> Do you expect all of this to be handled in userspace?
Yes, I do.
Thanks,
Richard
^ permalink raw reply
* [PATCH] net: ethernet: ti: netcp_core: no need in netif_napi_del
From: Ivan Khoronzhuk @ 2017-09-07 15:32 UTC (permalink / raw)
To: w-kwok2, m-karicheri2, netdev
Cc: linux-kernel, grygorii.strashko, Ivan Khoronzhuk
Don't remove rx_napi specifically just before free_netdev(),
it's supposed to be done in it and is confusing w/o tx_napi deletion.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
Based on net-next/master
drivers/net/ethernet/ti/netcp_core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index eb96a69..437d362 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -2145,7 +2145,6 @@ static void netcp_delete_interface(struct netcp_device *netcp_device,
of_node_put(netcp->node_interface);
unregister_netdev(ndev);
- netif_napi_del(&netcp->rx_napi);
free_netdev(ndev);
}
--
2.7.4
^ permalink raw reply related
* Re: [RFC net-next 0/5] TSN: Add qdisc-based config interfaces for traffic shapers
From: Henrik Austad @ 2017-09-07 15:27 UTC (permalink / raw)
To: Richard Cochran
Cc: Vinicius Costa Gomes, netdev, jhs, xiyou.wangcong, jiri,
intel-wired-lan, andre.guedes, ivan.briano,
jesus.sanchez-palencia, boon.leong.ong
In-Reply-To: <20170907124018.deinzo3c4ice3q7n@localhost>
[-- Attachment #1: Type: text/plain, Size: 3054 bytes --]
On Thu, Sep 07, 2017 at 02:40:18PM +0200, Richard Cochran wrote:
> On Thu, Sep 07, 2017 at 07:34:11AM +0200, Henrik Austad wrote:
> > Also, does this mean that when you create the qdisc, you have locked the
> > bandwidth for the scheduler? Meaning, if I later want to add another
> > stream that requires more bandwidth, I have to close all active streams,
> > reconfigure the qdisc and then restart?
>
> No, just allocate enough bandwidth to accomodate all of the expected
> streams. The streams can start and stop at will.
Sure, that'll work.
And if you want to this driver to act as a bridge, how do you accomodate
change in network requirements? (i.e. how does this work with switchdev?)
- Or am I overthinking this?
> > So my understanding of all of this is that you configure the *total*
> > bandwith for each class when you load the qdisc and then let userspace
> > handle the rest. Is this correct?
>
> Nothing wrong with that.
Didn't mean to say it was wrong, just making sure I've understood the
concept.
> > In my view, it would be nice if the qdisc had some notion about streams so
> > that you could create a stream, feed frames to it and let the driver pace
> > them out. (The fewer you queue, the shorter the delay). This will also
> > allow you to enforce per-stream bandwidth restrictions. I don't see how you
> > can do this here unless you want to do this in userspace.
> >
> > Do you have any plans for adding support for multiplexing streams? If you
> > have multiple streams, how do you enforce that one stream does not eat into
> > the bandwidth of another stream? AFAIK, this is something the network must
> > enforce, but I see no option of doing som here.
>
> Please, lets keep this simple.
Simple is always good
> Today we have exactly zero user space
> applications using this kind of bandwidth reservation. The case of
> wanting the kernel to police individual stream usage does not exist,
> and probably never will.
That we have *zero* userspace applications today is probably related to the
fact that we have exacatly *zero* drivers in the kernel that talks TSN :)
To rephrase a bit, what I'm worried about:
If you have more than 1 application in userspace that wants to send data
using this scheduler, how do you ensure fair transmission of frames? (both
how much bandwidth they use, but also ordering of frames from each
application) Do you expect all of this to be handled in userspace?
> For serious TSN use cases, the bandwidth needed by each system and
> indeed the entire network will be engineered, and we can reasonably
> expect applications to cooperate in this regard.
yes.. that'll happen ;)
> Thanks,
> Richard
Don't get me wrong, I think it is great that others are working on this!
I'm just trying to fully understand the thought that have gone into this
and how it is inteded to be used.
I'll get busy testing the code and wrapping my head around the different
parameters.
--
Henrik Austad
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] 9p: Fixes for hard-to-hit bugs
From: Latchesar Ionkov @ 2017-09-07 14:49 UTC (permalink / raw)
To: Tuomas Tynkkynen
Cc: Al Viro, V9FS Developers, Eric Van Hensbergen, Ron Minnich,
David S. Miller, Linux Kernel, netdev, linux-fsdevel
In-Reply-To: <20170906145908.8082-1-tuomas@tuxera.com>
Acked-by: Latchesar Ionkov <lucho@ionkov.net>
On Wed, Sep 6, 2017 at 8:59 AM, Tuomas Tynkkynen <tuomas@tuxera.com> wrote:
> These two patches fix two hard-to-hit (but really annoying) bugs in 9p.
> The first one was posted earlier in February (with one R-b), the second
> is a new one.
>
> Both of these have had soaking in NixOS distribution kernels for
> a couple of months with no ill effects.
>
> Tuomas Tynkkynen (2):
> fs/9p: Compare qid.path in v9fs_test_inode
> net/9p: Switch to wait_event_killable()
>
> fs/9p/vfs_inode.c | 3 +++
> fs/9p/vfs_inode_dotl.c | 3 +++
> net/9p/client.c | 3 +--
> net/9p/trans_virtio.c | 13 ++++++-------
> net/9p/trans_xen.c | 4 ++--
> 5 files changed, 15 insertions(+), 11 deletions(-)
>
> --
> 2.13.0
>
^ permalink raw reply
* Re: [PATCH] rtlwifi: btcoex: 23b 1ant: fix duplicated code for different branches
From: Gustavo A. R. Silva @ 2017-09-07 14:35 UTC (permalink / raw)
To: Larry Finger, Chaoming Li, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <ae09e952-112e-9616-735a-b86ba01ad398@lwfinger.net>
Hi Larry,
On 08/30/2017 11:48 PM, Larry Finger wrote:
> On 08/30/2017 08:42 AM, Gustavo A. R. Silva wrote:
>> Refactor code in order to avoid identical code for different branches.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Addresses-Coverity-ID: 1226788
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>> This issue was reported by Coverity and it was tested by compilation
>> only.
>> I'm suspicious this may be a copy/paste error. Please, verify.
>>
>> .../net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b1ant.c | 10
>> ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> This change is not correct. When bt_link_info->sco_exist is true, the
> call should be
>
> halbtc8723b1ant_limited_rx(btcoexist,
> NORMAL_EXEC, true,
> false, 0x5);
>
> NACK
>
> I will push the correct patch.
>
Great. Good to know.
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply
* Re: [V2 PATCH net-next 2/2] xdp: catch invalid XDP_REDIRECT API usage
From: Daniel Borkmann @ 2017-09-07 14:32 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, David S. Miller
Cc: Daniel Borkmann, John Fastabend, Andy Gospodarek,
alexei.starovoitov
In-Reply-To: <59B15403.2090109@iogearbox.net>
On 09/07/2017 04:13 PM, Daniel Borkmann wrote:
[...]
> + uint64_t addr = (unsigned long)prog;
And of course: s/uint64_t/u64/. Lack of context switch. ;-)
^ permalink raw reply
* RE: [PATCH net 0/3] lan78xx: Fixes to lan78xx driver
From: Woojung.Huh @ 2017-09-07 14:26 UTC (permalink / raw)
To: andrew; +Cc: Nisar.Sayed, davem, UNGLinuxDriver, netdev
In-Reply-To: <20170907141957.GL11248@lunn.ch>
> > > > This series of patches are for lan78xx driver.
> > > >
> > > > These patches fixes potential issues associated with lan78xx driver
> > >
> > > Hi Nisar
> > >
> > > So this is version 2? Please include v2 in the subject line.
> > >
> > > Also, briefly list what is different from the previous version.
> > Hi Andrew,
> >
> > Because Nisar dropped of non-mdio patch in the series,
> > I suggested to treat as new patch than version 2.
> > In this case, it is still considered as version 2 than new series?
>
> Hi Woojung
>
> So it is not a clear hard rule here. But we have seen these patches
> before, and they have been modified based on my comments. So i would
> say these are version 2. But a new series would also be O.K. What
> really matters is that the cover note explains what is going on. That
> some of the patches in the previous version have been dropped and will
> be posted later, and what changes have been made to the remaining
> patches.
Andrew,
Thanks for detail explanation and agree that cover page has more description.
Thanks.
Woojung
^ permalink raw reply
* [PATCH iproute2 3/4] devlink: Update devlink UAPI file
From: Arkadi Sharshevsky @ 2017-09-07 14:26 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, jiri, mlxsw, andrew, Arkadi Sharshevsky,
Jiri Pirko
In-Reply-To: <1504794403-45690-1-git-send-email-arkadis@mellanox.com>
Update devlink UAPI file.
Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/linux/devlink.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/linux/devlink.h b/include/linux/devlink.h
index 7644005..a62695e 100644
--- a/include/linux/devlink.h
+++ b/include/linux/devlink.h
@@ -226,4 +226,22 @@ enum devlink_dpipe_action_type {
DEVLINK_DPIPE_ACTION_TYPE_FIELD_MODIFY,
};
+enum devlink_dpipe_field_ethernet_id {
+ DEVLINK_DPIPE_FIELD_ETHERNET_DST_MAC,
+};
+
+enum devlink_dpipe_field_ipv4_id {
+ DEVLINK_DPIPE_FIELD_IPV4_DST_IP,
+};
+
+enum devlink_dpipe_field_ipv6_id {
+ DEVLINK_DPIPE_FIELD_IPV6_DST_IP,
+};
+
+enum devlink_dpipe_header_id {
+ DEVLINK_DPIPE_HEADER_ETHERNET,
+ DEVLINK_DPIPE_HEADER_IPV4,
+ DEVLINK_DPIPE_HEADER_IPV6,
+};
+
#endif /* _LINUX_DEVLINK_H_ */
--
2.4.11
^ permalink raw reply related
* [PATCH iproute2 0/4] Add support for dpipe's global header formatting
From: Arkadi Sharshevsky @ 2017-09-07 14:26 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, jiri, mlxsw, andrew, Arkadi Sharshevsky
Some dpipe's global header values need special formatting, for example
Ethernet and IP addresses. This patchset adds support for IPv4/6 and
Ethernet's special format.
Arkadi Sharshevsky (4):
devlink: Make match/action parsing more flexible
devlink: Add support for special format protocol headers
devlink: Update devlink UAPI file
devlink: Add support for protocol IPv4/IPv6/Ethernet special formats
devlink/devlink.c | 319 +++++++++++++++++++++++++++++++++++++-----------
include/linux/devlink.h | 18 +++
2 files changed, 265 insertions(+), 72 deletions(-)
--
2.4.11
^ permalink raw reply
* [PATCH iproute2 1/4] devlink: Make match/action parsing more flexible
From: Arkadi Sharshevsky @ 2017-09-07 14:26 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, jiri, mlxsw, andrew, Arkadi Sharshevsky,
Jiri Pirko
In-Reply-To: <1504794403-45690-1-git-send-email-arkadis@mellanox.com>
This patch decouples the match/action parsing from printing. This is
done as a preparation for adding the ability to print global header
values, for example print IPv4 address, which require special formatting.
Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 127 ++++++++++++++++++++++++++++++++++--------------------
1 file changed, 80 insertions(+), 47 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 8f11f86..36a2b36 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -3077,27 +3077,42 @@ static const char
}
}
-static void pr_out_dpipe_action(struct dpipe_ctx *ctx,
- uint32_t header_id, uint32_t field_id,
- uint32_t action_type, bool global)
+struct dpipe_op_info {
+ uint32_t header_id;
+ uint32_t field_id;
+ bool header_global;
+};
+
+struct dpipe_action {
+ struct dpipe_op_info info;
+ uint32_t type;
+};
+
+static void pr_out_dpipe_action(struct dpipe_action *action,
+ struct dpipe_ctx *ctx)
{
+ struct dpipe_op_info *op_info = &action->info;
const char *mapping;
- pr_out_str(ctx->dl, "type", dpipe_action_type_e2s(action_type));
- pr_out_str(ctx->dl, "header", dpipe_header_id2s(ctx, header_id,
- global));
- pr_out_str(ctx->dl, "field", dpipe_field_id2s(ctx, header_id, field_id,
- global));
- mapping = dpipe_mapping_get(ctx, header_id, field_id, global);
+ pr_out_str(ctx->dl, "type",
+ dpipe_action_type_e2s(action->type));
+ pr_out_str(ctx->dl, "header",
+ dpipe_header_id2s(ctx, op_info->header_id,
+ op_info->header_global));
+ pr_out_str(ctx->dl, "field",
+ dpipe_field_id2s(ctx, op_info->header_id,
+ op_info->field_id,
+ op_info->header_global));
+ mapping = dpipe_mapping_get(ctx, op_info->header_id,
+ op_info->field_id,
+ op_info->header_global);
if (mapping)
pr_out_str(ctx->dl, "mapping", mapping);
}
-static int dpipe_action_show(struct dpipe_ctx *ctx, struct nlattr *nl)
+static int dpipe_action_parse(struct dpipe_action *action, struct nlattr *nl)
{
struct nlattr *nla_action[DEVLINK_ATTR_MAX + 1] = {};
- uint32_t header_id, field_id, action_type;
- bool global;
int err;
err = mnl_attr_parse_nested(nl, attr_cb, nla_action);
@@ -3111,12 +3126,11 @@ static int dpipe_action_show(struct dpipe_ctx *ctx, struct nlattr *nl)
return -EINVAL;
}
- header_id = mnl_attr_get_u32(nla_action[DEVLINK_ATTR_DPIPE_HEADER_ID]);
- field_id = mnl_attr_get_u32(nla_action[DEVLINK_ATTR_DPIPE_FIELD_ID]);
- action_type = mnl_attr_get_u32(nla_action[DEVLINK_ATTR_DPIPE_ACTION_TYPE]);
- global = !!mnl_attr_get_u8(nla_action[DEVLINK_ATTR_DPIPE_HEADER_GLOBAL]);
+ action->type = mnl_attr_get_u32(nla_action[DEVLINK_ATTR_DPIPE_ACTION_TYPE]);
+ action->info.header_id = mnl_attr_get_u32(nla_action[DEVLINK_ATTR_DPIPE_HEADER_ID]);
+ action->info.field_id = mnl_attr_get_u32(nla_action[DEVLINK_ATTR_DPIPE_FIELD_ID]);
+ action->info.header_global = !!mnl_attr_get_u8(nla_action[DEVLINK_ATTR_DPIPE_HEADER_GLOBAL]);
- pr_out_dpipe_action(ctx, header_id, field_id, action_type, global);
return 0;
}
@@ -3124,16 +3138,18 @@ static int dpipe_table_actions_show(struct dpipe_ctx *ctx,
struct nlattr *nla_actions)
{
struct nlattr *nla_action;
+ struct dpipe_action action;
mnl_attr_for_each_nested(nla_action, nla_actions) {
pr_out_entry_start(ctx->dl);
- if (dpipe_action_show(ctx, nla_action))
- goto err_action_show;
+ if (dpipe_action_parse(&action, nla_action))
+ goto err_action_parse;
+ pr_out_dpipe_action(&action, ctx);
pr_out_entry_end(ctx->dl);
}
return 0;
-err_action_show:
+err_action_parse:
pr_out_entry_end(ctx->dl);
return -EINVAL;
}
@@ -3149,28 +3165,38 @@ dpipe_match_type_e2s(enum devlink_dpipe_match_type match_type)
}
}
-static void pr_out_dpipe_match(struct dpipe_ctx *ctx,
- uint32_t header_id, uint32_t field_id,
- uint32_t match_type, bool global)
+struct dpipe_match {
+ struct dpipe_op_info info;
+ uint32_t type;
+};
+
+static void pr_out_dpipe_match(struct dpipe_match *match,
+ struct dpipe_ctx *ctx)
{
+ struct dpipe_op_info *op_info = &match->info;
const char *mapping;
- pr_out_str(ctx->dl, "type", dpipe_match_type_e2s(match_type));
- pr_out_str(ctx->dl, "header", dpipe_header_id2s(ctx, header_id,
- global));
- pr_out_str(ctx->dl, "field", dpipe_field_id2s(ctx, header_id, field_id,
- global));
- mapping = dpipe_mapping_get(ctx, header_id, field_id, global);
+ pr_out_str(ctx->dl, "type",
+ dpipe_match_type_e2s(match->type));
+ pr_out_str(ctx->dl, "header",
+ dpipe_header_id2s(ctx, op_info->header_id,
+ op_info->header_global));
+ pr_out_str(ctx->dl, "field",
+ dpipe_field_id2s(ctx, op_info->header_id,
+ op_info->field_id,
+ op_info->header_global));
+ mapping = dpipe_mapping_get(ctx, op_info->header_id,
+ op_info->field_id,
+ op_info->header_global);
if (mapping)
pr_out_str(ctx->dl, "mapping", mapping);
-
}
-static int dpipe_match_show(struct dpipe_ctx *ctx, struct nlattr *nl)
+static int dpipe_match_parse(struct dpipe_match *match,
+ struct nlattr *nl)
+
{
struct nlattr *nla_match[DEVLINK_ATTR_MAX + 1] = {};
- uint32_t header_id, field_id, match_type;
- bool global;
int err;
err = mnl_attr_parse_nested(nl, attr_cb, nla_match);
@@ -3184,12 +3210,11 @@ static int dpipe_match_show(struct dpipe_ctx *ctx, struct nlattr *nl)
return -EINVAL;
}
- match_type = mnl_attr_get_u32(nla_match[DEVLINK_ATTR_DPIPE_MATCH_TYPE]);
- header_id = mnl_attr_get_u32(nla_match[DEVLINK_ATTR_DPIPE_HEADER_ID]);
- field_id = mnl_attr_get_u32(nla_match[DEVLINK_ATTR_DPIPE_FIELD_ID]);
- global = !!mnl_attr_get_u8(nla_match[DEVLINK_ATTR_DPIPE_HEADER_GLOBAL]);
+ match->type = mnl_attr_get_u32(nla_match[DEVLINK_ATTR_DPIPE_MATCH_TYPE]);
+ match->info.header_id = mnl_attr_get_u32(nla_match[DEVLINK_ATTR_DPIPE_HEADER_ID]);
+ match->info.field_id = mnl_attr_get_u32(nla_match[DEVLINK_ATTR_DPIPE_FIELD_ID]);
+ match->info.header_global = !!mnl_attr_get_u8(nla_match[DEVLINK_ATTR_DPIPE_HEADER_GLOBAL]);
- pr_out_dpipe_match(ctx, header_id, field_id, match_type, global);
return 0;
}
@@ -3197,16 +3222,18 @@ static int dpipe_table_matches_show(struct dpipe_ctx *ctx,
struct nlattr *nla_matches)
{
struct nlattr *nla_match;
+ struct dpipe_match match;
mnl_attr_for_each_nested(nla_match, nla_matches) {
pr_out_entry_start(ctx->dl);
- if (dpipe_match_show(ctx, nla_match))
- goto err_match_show;
+ if (dpipe_match_parse(&match, nla_match))
+ goto err_match_parse;
+ pr_out_dpipe_match(&match, ctx);
pr_out_entry_end(ctx->dl);
}
return 0;
-err_match_show:
+err_match_parse:
pr_out_entry_end(ctx->dl);
return -EINVAL;
}
@@ -3382,6 +3409,7 @@ static int dpipe_entry_match_value_show(struct dpipe_ctx *ctx,
struct nlattr *nl)
{
struct nlattr *nla_match_value[DEVLINK_ATTR_MAX + 1] = {};
+ struct dpipe_match match;
int err;
err = mnl_attr_parse_nested(nl, attr_cb, nla_match_value);
@@ -3394,16 +3422,18 @@ static int dpipe_entry_match_value_show(struct dpipe_ctx *ctx,
}
pr_out_entry_start(ctx->dl);
- if (dpipe_match_show(ctx, nla_match_value[DEVLINK_ATTR_DPIPE_MATCH]))
- goto err_match_show;
+ if (dpipe_match_parse(&match,
+ nla_match_value[DEVLINK_ATTR_DPIPE_MATCH]))
+ goto err_match_parse;
+ pr_out_dpipe_match(&match, ctx);
if (dpipe_entry_value_show(ctx, nla_match_value))
goto err_value_show;
pr_out_entry_end(ctx->dl);
return 0;
-err_match_show:
err_value_show:
+err_match_parse:
pr_out_entry_end(ctx->dl);
return -EINVAL;
}
@@ -3412,6 +3442,7 @@ static int dpipe_entry_action_value_show(struct dpipe_ctx *ctx,
struct nlattr *nl)
{
struct nlattr *nla_action_value[DEVLINK_ATTR_MAX + 1] = {};
+ struct dpipe_action action;
int err;
err = mnl_attr_parse_nested(nl, attr_cb, nla_action_value);
@@ -3424,16 +3455,18 @@ static int dpipe_entry_action_value_show(struct dpipe_ctx *ctx,
}
pr_out_entry_start(ctx->dl);
- if (dpipe_action_show(ctx, nla_action_value[DEVLINK_ATTR_DPIPE_ACTION]))
- goto err_action_show;
+ if (dpipe_action_parse(&action,
+ nla_action_value[DEVLINK_ATTR_DPIPE_ACTION]))
+ goto err_action_parse;
+ pr_out_dpipe_action(&action, ctx);
if (dpipe_entry_value_show(ctx, nla_action_value))
goto err_value_show;
pr_out_entry_end(ctx->dl);
return 0;
-err_action_show:
err_value_show:
+err_action_parse:
pr_out_entry_end(ctx->dl);
return -EINVAL;
}
--
2.4.11
^ permalink raw reply related
* [PATCH iproute2 2/4] devlink: Add support for special format protocol headers
From: Arkadi Sharshevsky @ 2017-09-07 14:26 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, jiri, mlxsw, andrew, Arkadi Sharshevsky,
Jiri Pirko
In-Reply-To: <1504794403-45690-1-git-send-email-arkadis@mellanox.com>
In case of global header (protocol header), the header:field ids are used
to perform lookup for special format printer. In case no printer existence
fallback to plain value printing.
Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 119 ++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 94 insertions(+), 25 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 36a2b36..b87de38 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -3372,9 +3372,89 @@ static int cmd_dpipe_table_set(struct dl *dl)
return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
}
-static int dpipe_entry_value_show(struct dpipe_ctx *ctx,
- struct nlattr **nla_match_value)
+enum dpipe_value_type {
+ DPIPE_VALUE_TYPE_VALUE,
+ DPIPE_VALUE_TYPE_MASK,
+};
+
+static const char *
+dpipe_value_type_e2s(enum dpipe_value_type type)
+{
+ switch (type) {
+ case DPIPE_VALUE_TYPE_VALUE:
+ return "value";
+ case DPIPE_VALUE_TYPE_MASK:
+ return "value_mask";
+ default:
+ return "<unknown>";
+ }
+}
+
+struct dpipe_field_printer {
+ unsigned int field_id;
+ void (*printer)(struct dpipe_ctx *, enum dpipe_value_type, void *);
+};
+
+struct dpipe_header_printer {
+ struct dpipe_field_printer *printers;
+ unsigned int printers_count;
+ unsigned int header_id;
+};
+
+static struct dpipe_header_printer *dpipe_header_printers[] = {};
+
+static int dpipe_print_prot_header(struct dpipe_ctx *ctx,
+ struct dpipe_op_info *info,
+ enum dpipe_value_type type,
+ void *value)
{
+ unsigned int header_printers_count = ARRAY_SIZE(dpipe_header_printers);
+ struct dpipe_header_printer *header_printer;
+ struct dpipe_field_printer *field_printer;
+ unsigned int field_printers_count;
+ int j;
+ int i;
+
+ for (i = 0; i < header_printers_count; i++) {
+ header_printer = dpipe_header_printers[i];
+ if (header_printer->header_id != info->header_id)
+ continue;
+ field_printers_count = header_printer->printers_count;
+ for (j = 0; j < field_printers_count; j++) {
+ field_printer = &header_printer->printers[j];
+ if (field_printer->field_id != info->field_id)
+ continue;
+ field_printer->printer(ctx, type, value);
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static void __pr_out_entry_value(struct dpipe_ctx *ctx,
+ void *value,
+ unsigned int value_len,
+ struct dpipe_op_info *info,
+ enum dpipe_value_type type)
+{
+ if (info->header_global &&
+ !dpipe_print_prot_header(ctx, info, type, value))
+ return;
+
+ if (value_len == sizeof(uint32_t)) {
+ uint32_t *value_32 = value;
+
+ pr_out_uint(ctx->dl, dpipe_value_type_e2s(type), *value_32);
+ }
+}
+
+static void pr_out_dpipe_entry_value(struct dpipe_ctx *ctx,
+ struct nlattr **nla_match_value,
+ struct dpipe_op_info *info)
+{
+ void *value, *value_mask;
+ uint32_t value_mapping;
uint16_t value_len;
bool mask, mapping;
@@ -3382,27 +3462,20 @@ static int dpipe_entry_value_show(struct dpipe_ctx *ctx,
mapping = !!nla_match_value[DEVLINK_ATTR_DPIPE_VALUE_MAPPING];
value_len = mnl_attr_get_payload_len(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE]);
- if (value_len == sizeof(uint32_t)) {
- uint32_t value, value_mask, value_mapping;
-
- if (mapping) {
- value_mapping = mnl_attr_get_u32(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE_MAPPING]);
- pr_out_uint(ctx->dl, "mapping_value", value_mapping);
- }
-
- if (mask) {
- value_mask = mnl_attr_get_u32(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE_MASK]);
- pr_out_uint(ctx->dl, "mask_value", value_mask);
- }
+ value = mnl_attr_get_payload(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE]);
- value = mnl_attr_get_u32(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE]);
- pr_out_uint(ctx->dl, "value", value);
+ if (mapping) {
+ value_mapping = mnl_attr_get_u32(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE_MAPPING]);
+ pr_out_uint(ctx->dl, "mapping_value", value_mapping);
+ }
- } else {
- return -EINVAL;
+ if (mask) {
+ value_mask = mnl_attr_get_payload(nla_match_value[DEVLINK_ATTR_DPIPE_VALUE]);
+ __pr_out_entry_value(ctx, value_mask, value_len, info,
+ DPIPE_VALUE_TYPE_MASK);
}
- return 0;
+ __pr_out_entry_value(ctx, value, value_len, info, DPIPE_VALUE_TYPE_VALUE);
}
static int dpipe_entry_match_value_show(struct dpipe_ctx *ctx,
@@ -3426,13 +3499,11 @@ static int dpipe_entry_match_value_show(struct dpipe_ctx *ctx,
nla_match_value[DEVLINK_ATTR_DPIPE_MATCH]))
goto err_match_parse;
pr_out_dpipe_match(&match, ctx);
- if (dpipe_entry_value_show(ctx, nla_match_value))
- goto err_value_show;
+ pr_out_dpipe_entry_value(ctx, nla_match_value, &match.info);
pr_out_entry_end(ctx->dl);
return 0;
-err_value_show:
err_match_parse:
pr_out_entry_end(ctx->dl);
return -EINVAL;
@@ -3459,13 +3530,11 @@ static int dpipe_entry_action_value_show(struct dpipe_ctx *ctx,
nla_action_value[DEVLINK_ATTR_DPIPE_ACTION]))
goto err_action_parse;
pr_out_dpipe_action(&action, ctx);
- if (dpipe_entry_value_show(ctx, nla_action_value))
- goto err_value_show;
+ pr_out_dpipe_entry_value(ctx, nla_action_value, &action.info);
pr_out_entry_end(ctx->dl);
return 0;
-err_value_show:
err_action_parse:
pr_out_entry_end(ctx->dl);
return -EINVAL;
--
2.4.11
^ permalink raw reply related
* [PATCH iproute2 4/4] devlink: Add support for protocol IPv4/IPv6/Ethernet special formats
From: Arkadi Sharshevsky @ 2017-09-07 14:26 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, jiri, mlxsw, andrew, Arkadi Sharshevsky,
Jiri Pirko
In-Reply-To: <1504794403-45690-1-git-send-email-arkadis@mellanox.com>
Add support for protocol IPv4/IPv6/Ethernet special formats.
Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 74 insertions(+), 1 deletion(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index b87de38..39cda06 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -20,6 +20,7 @@
#include <linux/genetlink.h>
#include <linux/devlink.h>
#include <libmnl/libmnl.h>
+#include <netinet/ether.h>
#include "SNAPSHOT.h"
#include "list.h"
@@ -3401,7 +3402,79 @@ struct dpipe_header_printer {
unsigned int header_id;
};
-static struct dpipe_header_printer *dpipe_header_printers[] = {};
+static void dpipe_field_printer_ipv4_addr(struct dpipe_ctx *ctx,
+ enum dpipe_value_type type,
+ void *value)
+{
+ struct in_addr ip_addr;
+
+ ip_addr.s_addr = htonl(*(uint32_t *)value);
+ pr_out_str(ctx->dl, dpipe_value_type_e2s(type), inet_ntoa(ip_addr));
+}
+
+static void
+dpipe_field_printer_ethernet_addr(struct dpipe_ctx *ctx,
+ enum dpipe_value_type type,
+ void *value)
+{
+ pr_out_str(ctx->dl, dpipe_value_type_e2s(type),
+ ether_ntoa((struct ether_addr *)value));
+}
+
+static void dpipe_field_printer_ipv6_addr(struct dpipe_ctx *ctx,
+ enum dpipe_value_type type,
+ void *value)
+{
+ char str[INET6_ADDRSTRLEN];
+
+ inet_ntop(AF_INET6, value, str, INET6_ADDRSTRLEN);
+ pr_out_str(ctx->dl, dpipe_value_type_e2s(type), str);
+}
+
+static struct dpipe_field_printer dpipe_field_printers_ipv4[] = {
+ {
+ .printer = dpipe_field_printer_ipv4_addr,
+ .field_id = DEVLINK_DPIPE_FIELD_IPV4_DST_IP,
+ }
+};
+
+static struct dpipe_header_printer dpipe_header_printer_ipv4 = {
+ .printers = dpipe_field_printers_ipv4,
+ .printers_count = ARRAY_SIZE(dpipe_field_printers_ipv4),
+ .header_id = DEVLINK_DPIPE_HEADER_IPV4,
+};
+
+static struct dpipe_field_printer dpipe_field_printers_ethernet[] = {
+ {
+ .printer = dpipe_field_printer_ethernet_addr,
+ .field_id = DEVLINK_DPIPE_FIELD_ETHERNET_DST_MAC,
+ },
+};
+
+static struct dpipe_header_printer dpipe_header_printer_ethernet = {
+ .printers = dpipe_field_printers_ethernet,
+ .printers_count = ARRAY_SIZE(dpipe_field_printers_ethernet),
+ .header_id = DEVLINK_DPIPE_HEADER_ETHERNET,
+};
+
+static struct dpipe_field_printer dpipe_field_printers_ipv6[] = {
+ {
+ .printer = dpipe_field_printer_ipv6_addr,
+ .field_id = DEVLINK_DPIPE_FIELD_IPV6_DST_IP,
+ }
+};
+
+static struct dpipe_header_printer dpipe_header_printer_ipv6 = {
+ .printers = dpipe_field_printers_ipv6,
+ .printers_count = ARRAY_SIZE(dpipe_field_printers_ipv6),
+ .header_id = DEVLINK_DPIPE_HEADER_IPV6,
+};
+
+static struct dpipe_header_printer *dpipe_header_printers[] = {
+ &dpipe_header_printer_ipv4,
+ &dpipe_header_printer_ethernet,
+ &dpipe_header_printer_ipv6,
+};
static int dpipe_print_prot_header(struct dpipe_ctx *ctx,
struct dpipe_op_info *info,
--
2.4.11
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox