* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 13:10 UTC (permalink / raw)
To: Yi Yang
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA, e,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <1504096752-102003-1-git-send-email-yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On Wed, 30 Aug 2017 20:39:12 +0800, Yi Yang wrote:
> +enum ovs_nsh_key_attr {
> + OVS_NSH_KEY_ATTR_BASE, /* struct ovs_nsh_key_base. */
> + OVS_NSH_KEY_ATTR_MD1, /* struct ovs_nsh_key_md1. */
> + OVS_NSH_KEY_ATTR_MD2, /* variable-length octets for MD type 2. */
> + __OVS_NSH_KEY_ATTR_MAX
> +};
One more thing: 0 is reserved, the first attr must be
OVS_NSH_KEY_ATTR_UNSPEC.
Jiri
^ permalink raw reply
* RE: [PATCH net-next v7] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 12:57 UTC (permalink / raw)
To: Jiri Benc, Yang, Yi
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org
In-Reply-To: <20170904124216.6db68e8c@griffin>
> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:
> > I think we have had similiar discussion about this, the issue is we have
> > no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
> >
> > So we have to do so, there is miniflow to handle such issue in
> > userspace, but kernel data path didn't provide such mechanism. I think
> > every newly-added key will result in the same space-wasting issue for
> > kernel data path, isn't it?
>
> Yes. And it would be surprising if it didn't have an effect on
> performance.
>
> I don't care that much as this is a result of openvswitch module design
> decisions but it still would be good to reach a consensus before
> implementing uAPI that might not be needed in the end.
Matching on NSH MD1 context headers is definitely required. So these must
be part of the flow.
>
> > OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
> > action, so no reference code for this, set action has two use cases, one
> > has mask but the other hasn't, so it will be easier an nested attribute is
> > handled as a whole, in user space, nsh_key_from_nlattr is used in
> > several places.
>
> I very much disagree. What you're doing is very poor design as can be
> seen from the code where you have to do weird gymnastics to implement
> that. Compare that to a much simple case where each attribute carries
> its own value and mask.
I have no principle objections against splitting NSH base header and MD1 context
headers into independent key attributes on the uAPI if that simplifies the code.
But we need to full picture here:
So is what you are suggesting the following?
For matching:
OVS_KEY_ATTR_NSH_BASE_HEADER mandatory
OVS_KEY_ATTR_NSH_MD1_CONTEXT optional, in case MDTYPE == MD1
For setting:
OVS_ACTION_ATTR_SET_MASKED
- OVS_KEY_ATTR_NSH_BASE_HEADER when setting any base header fields
OVS_ACTION_ATTR_SET_MASKED
- OVS_KEY_ATTR_NSH_MD1_CONTEXT when setting any MD1 context data fields
For push_nsh:
OVS_ACTION_ATTR_PUSH_NSH
- OVS_KEY_ATTR_NSH_BASE_HEADER mandatory
- OVS_NSH_ATTR_CONTEXT_HEADERS optional (opaque metadata octet stream)
So push_nsh would still require nested attributes. Is that what we want (see below).
> > How can we do so? I see batch process code in user space implementation,
> > but kernel data path didn't such infrastructure,
>
> You're right that there's no infrastructure. I somewhat agree that it
> might be out of scope of this patch and it can be optimized later.
> That's why I also included other suggestions to speed this up until we
> implement better parsing: namely, verify correctness once (at the time
> it is set from user space) and just expect things to be correct in the
> fast path.
>
> > how can we know next push_nsh uses the same nsh header as previous
> > one?
>
> We store the prepopulated header together with the action.
I agree.
In our original modelling of OVS_ACTION_ATTR_PUSH_NSH in OVS 2.8 we
introduced a monolithic push_nsh action struct including the NSH base header
and variable length opaque context headers. That avoids any logic in the
kernel datapath to translate a nested action attribute into an internal format
with pre-populated header. The user space does that work.
Should we consider to stick to that simple and efficient approach? As far
As I can see it will be generic for the foreseeable future.
> > > These checks should be done only once when the action is configured, not for
> > > each packet.
> >
> > I don't know how to implement a batch processing in kernel data path, it
> > seems there isn't such code for reference.
>
> The checks should be done somewhere in __ovs_nla_copy_action. Which
> seems to be done even in your patch by nsh_key_put_from_nlattr
> (I didn't get that far during the review last week. The names of
> those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to
> tell what they do without looking at the call sites - something you
> should also consider to improve). That makes it even more wrong: you
> appear to check everything twice, first on configuration and then for
> every packet.
Agree. Validation should only happen at action configuration time.
BR, Jan
^ permalink raw reply
* stable-kernel-rules: wireless and netdev-FAQ
From: Kalle Valo @ 2017-09-04 12:58 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Larry Finger, Helmut Schaa, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170902082223.GA11293@redhat.com>
(adding netdev and lkml)
Stanislaw Gruszka <sgruszka@redhat.com> writes:
> On Fri, Sep 01, 2017 at 05:31:57PM +0300, Kalle Valo wrote:
>> Stanislaw Gruszka <sgruszka@redhat.com> writes:
>>
>> > On Thu, Aug 31, 2017 at 10:33:28AM -0500, Larry Finger wrote:
>> >> Should the patch to wireless-drivers be annotated with a Stable reference so
>> >> that it is added to 4.12 and 4.13?
>> >
>> > According to Documentation/networking/netdev-FAQ.txt networking patches
>> > should not be marked cc:stable, instead a decent commit log should
>> > be written describing a bugfix. Which I believe it is done for
>> > this patch.
>>
>> But that's for net and net-next trees, not for wireless trees. With
>> wireless patches we use "Cc: stable@..." references.
>
> Oh, ok. I was confused by below part of
> Documentation/process/stable-kernel-rules.rst
> (because wireless drivers are located in drivers/net/)
>
> - If the patch covers files in net/ or drivers/net please follow netdev stable
> submission guidelines as described in
> Documentation/networking/netdev-FAQ.txt
Yeah, that's confusing and should be clarified that wireless follows
traditional stable process. IIRC bluetooth does the same so it's not
just wireless.
Any volunteers to fix it? :)
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 12:57 UTC (permalink / raw)
To: Yang, Yi
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170904120907.GA75461@cran64.bj.intel.com>
On Mon, 4 Sep 2017 20:09:07 +0800, Yang, Yi wrote:
> So we must do many changes if we want to break this assumption.
We may do as many changes as we want to. This is uAPI we're talking
about and we need to get it right since the beginning. Sure, it may
mean that some user space programs need some changes in order to make
use of the new features. That happens every day.
I also don't understand where's the problem. It's very easy to check
for NLA_F_NESTED and generically act based on that in the function you
quoted. Just call a different function than format_odp_key_attr to
handle ovs_nsh_key_attr attributes in case the nested flag is set and
the attribute is OVS_KEY_ATTR_NSH and you're done. You'll need such
function anyway, it's not much different code size wise to call it from
format_odp_key_attr or from format_odp_action.
Jiri
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Yang, Yi @ 2017-09-04 12:09 UTC (permalink / raw)
To: Jiri Benc
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170904124216.6db68e8c@griffin>
On Mon, Sep 04, 2017 at 12:42:16PM +0200, Jiri Benc wrote:
> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:
> > I think we have had similiar discussion about this, the issue is we have
> > no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
> >
> > So we have to do so, there is miniflow to handle such issue in
> > userspace, but kernel data path didn't provide such mechanism. I think
> > every newly-added key will result in the same space-wasting issue for
> > kernel data path, isn't it?
>
> Yes. And it would be surprising if it didn't have an effect on
> performance.
>
> I don't care that much as this is a result of openvswitch module design
> decisions but it still would be good to reach a consensus before
> implementing uAPI that might not be needed in the end.
>
> > OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
> > action, so no reference code for this, set action has two use cases, one
> > has mask but the other hasn't, so it will be easier an nested attribute is
> > handled as a whole, in user space, nsh_key_from_nlattr is used in
> > several places.
>
> I very much disagree. What you're doing is very poor design as can be
> seen from the code where you have to do weird gymnastics to implement
> that. Compare that to a much simple case where each attribute carries
> its own value and mask.
>
> > If we change it per your proposal, there will be a lot
> > of rework,
>
> That's not an argument. We care about doing things right, we don't do
> things hastily and with as little effort as possible.
Jiri, I check OVS source code carefully, if you check OVS code in
format_odp_action in lib/odp-util.c, it has a strong assumption for set
action, mask is following value no matter it is simple attribute or
nested attribute, I copy source code here for your reference,
case OVS_ACTION_ATTR_SET_MASKED:
a = nl_attr_get(a);
size = nl_attr_get_size(a) / 2;
ds_put_cstr(ds, "set(");
/* Masked set action not supported for tunnel key, which is
* bigger. */
if (size <= sizeof(struct ovs_key_ipv6)) {
struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct
ovs_key_ipv6),
sizeof(struct nlattr))];
struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct
ovs_key_ipv6),
sizeof(struct nlattr))];
mask->nla_type = attr->nla_type = nl_attr_type(a);
mask->nla_len = attr->nla_len = NLA_HDRLEN + size;
memcpy(attr + 1, (char *)(a + 1), size);
memcpy(mask + 1, (char *)(a + 1) + size, size);
format_odp_key_attr(attr, mask, NULL, ds, false);
} else {
format_odp_key_attr(a, NULL, NULL, ds, false);
}
ds_put_cstr(ds, ")");
break;
So we must do many changes if we want to break this assumption.
>
> > we have to provide two functions to handle this, one is for
> > the case with mask, the other is for the case without mask.
>
> I don't see that. The function is called from a single place only.
>
> > How can we do so? I see batch process code in user space implementation,
> > but kernel data path didn't such infrastructure,
>
> You're right that there's no infrastructure. I somewhat agree that it
> might be out of scope of this patch and it can be optimized later.
> That's why I also included other suggestions to speed this up until we
> implement better parsing: namely, verify correctness once (at the time
> it is set from user space) and just expect things to be correct in the
> fast path.
>
> > how can we know next push_nsh uses the same nsh header as previous
> > one?
>
> We store the prepopulated header together with the action.
>
> > > Shouldn't we reject the packet, then? Pretending that something was parsed
> > > correctly while in fact it was not, does not look correct.
> >
> > For MD type 2, we don't implement metadata parsing, but it can still
> > work. I'm not sure what you mean here, how do we reject a packet here?
>
> Okay, we can match on mdtype and thus can find out about this type of
> packets. No problem, then.
>
> > > These checks should be done only once when the action is configured, not for
> > > each packet.
> >
> > I don't know how to implement a batch processing in kernel data path, it
> > seems there isn't such code for reference.
>
> The checks should be done somewhere in __ovs_nla_copy_action. Which
> seems to be done even in your patch by nsh_key_put_from_nlattr
> (I didn't get that far during the review last week. The names of
> those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to
> tell what they do without looking at the call sites - something you
> should also consider to improve). That makes it even more wrong: you
> appear to check everything twice, first on configuration and then for
> every packet.
Ok, I'll carefully remove unnecessary checks in next version.
>
> Jiri
^ permalink raw reply
* RE: [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 11:57 UTC (permalink / raw)
To: Hannes Frederic Sowa, Yang, Yi
Cc: netdev@vger.kernel.org, dev@openvswitch.org, jbenc@redhat.com,
e@erig.me, blp@ovn.org
In-Reply-To: <87mv6abte5.fsf@stressinduktion.org>
> >> Does it makes sense to keep the context headers as part of the flow?
> >> What is the reasoning behind it? With mdtype 2 headers this might either
> >> not work very well or will increase sw_flow_key size causing slowdowns
> >> for all protocols.
> >
> > For userspace, miniflow can handle such issue, but kernel data path
> > didn't provide such mechanism, so I don't think we can think of a better
> > way to fix this.
>
> I do think that both, kernel and dpdk dp, are fine if you don't match on
> context fields, which I think is the common case.
Agree
> As soon as a few paths start to match on contexts at different offsets I
> am not sure if miniflow will actually help. I don't know enough about
> that. Set cover is a NP hard problem, so constructing plain simple flows
> will be an approximation somewhere anyway at some point.
>
> If the context values are in some way discrete it might make sense, but
> for load balancing purposes your ingress router might fill out one
> context field with a flow hash of the inner flow to allow ECMP or
> loadbalancing. You might want to treat that separately. Otherwise the
> different paths might always need to context[3] & 0x3 (bits as number of
> possible next hops) and put that into the flow state. Because every hop
> and every path might have different numbers of nexthops etc., I don't
> think this is easy unifiable anymore.
Yes, that would become a scaling challenge for the datapaths and also for
schemes to off-load that datapath to HW. I believe it requires discipline
on the controller side to not let the number of needed masks explode for
OVS.
> > I believe every newly-added key will result in similiar issue you
> > concern, maybe it will be better to introduce miniflow in kernel data
> > path, but it isn't in scope of this patch series.
>
> You will see the same problem with offloading flows e.g. to network
> cards very soon. Flow explosion here will cause your 100Gbit/s NIC
> suddenly to go to software slow path.
>
> > It will be great if you can have a better proposal to fix your concern.
>
> I do think that something alike miniflow might make sense, but I don't
> know enough about miniflow.
Struct miniflow in the netdev datapath is merely a compact representation
of struct flow. struct flow is partitioned in to 64-bit "stripes" and struct
miniflow only stores those 64-bit bit stripes that have a non-zero mask stripe.
It reduces the memory footprint for storing packet flow and megaflow entries
but does not provide any megaflow lookup performance as such.
> New ACTIONS, that are not only bound to NSH, seem appealing to me at the
> moment. E.g. have a subtable match in the kernel dp or allowing for some
> OXM % modulo -> select action or neighbor alike thing would probably
> cover a lot of cases in the beginning. The submatch would probably very
> much reassemble the miniflow in dpdk dp.
I would like to discuss your ideas to optimize scalable load-sharing in OVS.
But I think we should do that in a separate mail thread to let the immediate
NSH kernel datapath work proceed.
Can you provide some more details of what you suggest? Perhaps take that
on the ovs-dev mailing list.
BR, Jan
^ permalink raw reply
* Re: [ethtool] ethtool: Remove UDP Fragmentation Offload use from ethtool
From: Tariq Toukan @ 2017-09-04 11:46 UTC (permalink / raw)
To: Michal Kubecek, Tariq Toukan
Cc: John W. Linville, Eric Dumazet, David Miller, netdev,
Eran Ben Elisha, Shaker Daibes
In-Reply-To: <20170829104452.b6n5ikxxioib7766@unicorn.suse.cz>
On 29/08/2017 1:44 PM, Michal Kubecek wrote:
> On Tue, Aug 29, 2017 at 10:50:20AM +0300, Tariq Toukan wrote:
>> On 28/08/2017 9:22 PM, John W. Linville wrote:
>>> On Mon, Aug 28, 2017 at 08:00:11AM -0700, Eric Dumazet wrote:
>>>> On Mon, 2017-08-28 at 15:38 +0300, Tariq Toukan wrote:
>>>>> From: Shaker Daibes <shakerd@mellanox.com>
>>>>>
>>>>> UFO was removed in kernel, here we remove it in ethtool app.
>>>>>
>>>>> Fixes the following issue:
>>>>> Features for ens8:
>>>>> Cannot get device udp-fragmentation-offload settings: Operation not supported
>>
>> But I wonder how the warning removal should be done??
>>
>> I have some suggestions in mind:
>> 1) Have a special condition that does not print a warning only in the case
>> of UFO?
>> 2) Remove the warning totally? I don't like this option.
>> 3) Add a max_kernel_ver field in struct off_flag_def, and use it to not
>> print the warning, or to mark the feature 'off [fixed]'.
>
> IMHO there is nothing wrong with not writing a warning for "get"
> operation, after all it's just "ethtool -k", i.e. "show me all
> offloading flags" and we do not warn about unsupported named features
> either. IMHO the only question should be how friendly we should be to
> old scripts expecting the line in the output:
>
> (a) omit the "udp-fragmentation-offload:" line (be consistent)
> (b) say something like "udp-fragmentation-offload: n/a"
> (c) pretend it's there and is off (most careful but misleading)
>
> Personally, I would prefer (a) but some badly written scripts might have
> problem with this approach.
>
Thanks Michal.
I think we'll go for some variation of (a), omitting the warning.
Here we can choose to limit the omission for this exact case, i.e. only
when offload is UFO and err is "Operation not supported".
> On the other hand, an attempt to _set_ the flag with "ethtool -K" should
> issue an error.
>
> Michal Kubecek
>
^ permalink raw reply
* Re: [ovs-dev] [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Hannes Frederic Sowa @ 2017-09-04 11:45 UTC (permalink / raw)
To: Jan Scheurich
Cc: Mooney, Sean K, dev@openvswitch.org, e@erig.me, jbenc@redhat.com,
netdev@vger.kernel.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D787F4CB0@ESESSMB107.ericsson.se>
Hello,
Jan Scheurich <jan.scheurich@ericsson.com> writes:
>> >> >> Does it makes sense to keep the context headers as part of the flow?
>> >> >> What is the reasoning behind it? With mdtype 2 headers this might
>> >> >> either not work very well or will increase sw_flow_key size causing
>> >> >> slowdowns for all protocols.
>> >> > [Mooney, Sean K]
>> >> > Having the nsh context headers in the flow is quite useful It would
>> >> > allow loadblancing on values stored in the context headers Or other
>> >> > use. I belive odl previously used context header 4 to store a Flow id
>> >> > so this could potentialy be used with the multipath action to have
>> >> ovs
>> >> > Choose between several possible next hops in the chain.
>> >>
>> >> In OVS, masks are a list(!) for matching. How can this work for
>> >> different paths that might require different masks? If they can't be
>> >> unified you even get exact matches. Thus, for OVS the context should
>> >> not be part of the flow.
>>
>> > [Mooney, Sean K] I'm not sure what you mean about a list as I never
>> > made reference to one. md type 1 context headers are 4 mandatory 32
>> > bit field.
>>
>> The semantic of the context fields depend on the path id. Every path can
>> have their own interpretation of context fields.
>>
>> Thus the paths will cetainly have their own masks. I hope I understood
>> this part of the standard correctly.
>>
>> dp only supports installing (approximated) disjoint megaflows and this
>> affects performance a lot. Every new mask that gets added to the dp will
>> be installed into a list which will be walked sequentially for
>> packets. This will kill performance.
>>
>> I assume that user space slows down, too, depending on the algorithm
>> they use generate megaflows.
>
> The primary use case for OVS in SFC/NSH is SFF. In almost all SFF use
> cases OVS will not need to match on context headers. The ability of OVS
> to match on context headers should not in general slow down the datapath.
The sw_flow_key is huge nowadays. You will be expanding it to the eigth
cache line. But I agree that it should not generally slow down the data
path.
> When SFC controllers match on parts of the context headers (e.g. in the
> final SFF or for load-balancing purposes), we will get additional megaflow
> masks. This is a fact of life in OVS and nothing new in NSH. I don't think
> this should prevent us from supporting valid use cases in OVS.
Other protocols are using entropy from the protocol and load balance on
that implicitly. Why not NSH?
I would argue that before NSH the masks were pretty constant. NSH
introduces context dependent paths where the design emphasizes to have
different masks per tenant. I don't think this is the case for other
protocols processed by the kernel dp right now. So the megaflow
unification was rather effective so far.
I expect a major slow down with just 10-20 masks.
Btw. this also affects possibilities to synthesize those flows to
hardware at all.
> The slow-down of the megaflow lookup in the datapath with the number
> of masks has been addressed in the user-space datapath with sorting the
> mask lists according to hit frequency and maintaining one sorted lists per
> ingress port. There is further work in progress to improve on this with a
> cuckoo hash to pick the most likely mask first.
> It should be possible to apply similar optimization schemes also in the
> Kernel datapath.
Lots of optimizations could be done, I agree, but the fundamental
problem still exists, especially if you want to be traffic agnostic
(short slow lived flows, 64 byte size UDP/RTP traffic etc., you
basically walk through more layers of abstraction to find your flow
resolution).
>
>> > form an ovs context they should be treated the same as the 32 bit register fields.
>> > We do not need to necessarily support those in md type 2 as all metadata is optional.
>
> Support for matching on or updating MD2 TLV context headers is FFS in a
> future OVS release. We do not foresee to add MD2 TLVs explicitly to the
> flow struct.
In my opinion I don't see a difference between MD1 and MD2.
>> > You can also see that context header 1 and 2 are still used in the newer odl netvirt sfc classifier implementation
>> > https://github.com/opendaylight/netvirt/blob/ba22f7cf19d8a827d77a3391a7f654344ade43d8/docs/specs/new-sfc-
>> classifier.rst#pipeline-changes
>> > so for odl existing nsh support to work with md type one we must have the ability to set the nsh context headers
>> > and should have the ability to match on them also for load lancing between service function in service function group.
>>
>> As I said, a separate action sounds much more useful.
>
> I don't think it is a good approach in OVS to introduce custom actions
> for NSH, e.g. for load sharing based on context header data. The
> primary tool for load sharing in OpenFlow is the Select Group. OVS
> already support an extension to the OF 1.5 Group Mod message to
> specify which fields to hash on. This can be used to hash on the
> relevant NSH context headers.
It doesn't need to be custom to NSH at all. A load balancing action
based on a flow hash seems pretty common to me (I am talking about
kernel actions here).
Thanks and bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/2] netfilter/xt_hashlimit: new feature/algorithm for xt_hashlimit
From: Pablo Neira Ayuso @ 2017-09-04 11:31 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: kadlec, netfilter-devel, johunt, fw, netdev, pai.vishwain
In-Reply-To: <20170904101433.GA3979@salvia>
On Mon, Sep 04, 2017 at 12:14:33PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 18, 2017 at 04:58:59PM -0400, Vishwanath Pai wrote:
> [...]
> > The main difference between the existing algorithm and the new one is
> > that the existing algorithm rate-limits the flow whereas the new
> > algorithm does not. Instead it *classifies* the flow based on whether
> > it is above or below a certain rate. I will demonstrate this with an
> > example below. Let us assume this rule:
> >
> > iptables -A INPUT -m hashlimit --hashlimit-above 10/s -j new_chain
> >
> > If the packet rate is 15/s, the existing algorithm would ACCEPT 10
> > packets every second and send 5 packets to "new_chain".
> >
> > But with the new algorithm, as long as the rate of 15/s is sustained,
> > all packets will continue to match and every packet is sent to new_chain.
>
> Sounds good, applied, thanks.
BTW, for the record, I have rename this patch title to:
netfilter: xt_hashlimit: add rate match mode
^ permalink raw reply
* Re: [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Hannes Frederic Sowa @ 2017-09-04 11:22 UTC (permalink / raw)
To: Yang, Yi
Cc: netdev@vger.kernel.org, dev@openvswitch.org, jbenc@redhat.com,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170904023831.GA68062@cran64.bj.intel.com>
Hello,
"Yang, Yi" <yi.y.yang@intel.com> writes:
> On Wed, Aug 30, 2017 at 05:53:27PM +0800, Hannes Frederic Sowa wrote:
>> Hello,
>>
>> Yi Yang <yi.y.yang@intel.com> writes:
>>
>> [...]
>>
>> > +struct ovs_key_nsh {
>> > + u8 flags;
>> > + u8 ttl;
>> > + u8 mdtype;
>> > + u8 np;
>> > + __be32 path_hdr;
>> > + __be32 context[NSH_MD1_CONTEXT_SIZE];
>> > +};
>> > +
>> > struct sw_flow_key {
>> > u8 tun_opts[IP_TUNNEL_OPTS_MAX];
>> > u8 tun_opts_len;
>> > @@ -144,6 +154,7 @@ struct sw_flow_key {
>> > };
>> > } ipv6;
>> > };
>> > + struct ovs_key_nsh nsh; /* network service header */
>> > struct {
>> > /* Connection tracking fields not packed above. */
>> > struct {
>>
>> Does it makes sense to keep the context headers as part of the flow?
>> What is the reasoning behind it? With mdtype 2 headers this might either
>> not work very well or will increase sw_flow_key size causing slowdowns
>> for all protocols.
>
> For userspace, miniflow can handle such issue, but kernel data path
> didn't provide such mechanism, so I don't think we can think of a better
> way to fix this.
I do think that both, kernel and dpdk dp, are fine if you don't match on
context fields, which I think is the common case.
As soon as a few paths start to match on contexts at different offsets I
am not sure if miniflow will actually help. I don't know enough about
that. Set cover is a NP hard problem, so constructing plain simple flows
will be an approximation somewhere anyway at some point.
If the context values are in some way discrete it might make sense, but
for load balancing purposes your ingress router might fill out one
context field with a flow hash of the inner flow to allow ECMP or
loadbalancing. You might want to treat that separately. Otherwise the
different paths might always need to context[3] & 0x3 (bits as number of
possible next hops) and put that into the flow state. Because every hop
and every path might have different numbers of nexthops etc., I don't
think this is easy unifiable anymore.
> We have to have context headers in flow for match and set, every hop in
> service function chaining can put its metedata into context headers on
> demand, MD type 2 is much more complicated than you can imagine, it is
> impossible to use an uniform way to handle MD type 1 and MD type 2, our
> way is to keep MD type 1 keys in struct ovs_key_nsh and to reuse struct
> flow_tnl for MD type 2 metadata, in case of MD type 2, we can set
> context headers to 0 in struct ovs_key_nsh.
Understood and agreed.
> I beleive every newly-added key will result in similiar issue you
> concern, maybe it will be better to introduce miniflow in kernel data
> path, but it isn't in scope of this patch series.
You will see the same problem with offloading flows e.g. to network
cards very soon. Flow explosion here will cause your 100Gbit/s NIC
suddenly to go to software slow path.
> It will be great if you can have a better proposal to fix your concern.
I do think that something alike miniflow might make sense, but I don't
know enough about miniflow.
New ACTIONS, that are not only bound to NSH, seem appealing to me at the
moment. E.g. have a subtable match in the kernel dp or allowing for some
OXM % modulo -> select action or neighbor alike thing would probably
cover a lot of cases in the beginning. The submatch would probably very
much reassemble the miniflow in dpdk dp.
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 10:42 UTC (permalink / raw)
To: Yang, Yi
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20170904080005.GA70767-re2EX8HDrk21gSHoDXDV2kEOCMrvLtNR@public.gmane.org>
On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:
> I think we have had similiar discussion about this, the issue is we have
> no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
>
> So we have to do so, there is miniflow to handle such issue in
> userspace, but kernel data path didn't provide such mechanism. I think
> every newly-added key will result in the same space-wasting issue for
> kernel data path, isn't it?
Yes. And it would be surprising if it didn't have an effect on
performance.
I don't care that much as this is a result of openvswitch module design
decisions but it still would be good to reach a consensus before
implementing uAPI that might not be needed in the end.
> OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
> action, so no reference code for this, set action has two use cases, one
> has mask but the other hasn't, so it will be easier an nested attribute is
> handled as a whole, in user space, nsh_key_from_nlattr is used in
> several places.
I very much disagree. What you're doing is very poor design as can be
seen from the code where you have to do weird gymnastics to implement
that. Compare that to a much simple case where each attribute carries
its own value and mask.
> If we change it per your proposal, there will be a lot
> of rework,
That's not an argument. We care about doing things right, we don't do
things hastily and with as little effort as possible.
> we have to provide two functions to handle this, one is for
> the case with mask, the other is for the case without mask.
I don't see that. The function is called from a single place only.
> How can we do so? I see batch process code in user space implementation,
> but kernel data path didn't such infrastructure,
You're right that there's no infrastructure. I somewhat agree that it
might be out of scope of this patch and it can be optimized later.
That's why I also included other suggestions to speed this up until we
implement better parsing: namely, verify correctness once (at the time
it is set from user space) and just expect things to be correct in the
fast path.
> how can we know next push_nsh uses the same nsh header as previous
> one?
We store the prepopulated header together with the action.
> > Shouldn't we reject the packet, then? Pretending that something was parsed
> > correctly while in fact it was not, does not look correct.
>
> For MD type 2, we don't implement metadata parsing, but it can still
> work. I'm not sure what you mean here, how do we reject a packet here?
Okay, we can match on mdtype and thus can find out about this type of
packets. No problem, then.
> > These checks should be done only once when the action is configured, not for
> > each packet.
>
> I don't know how to implement a batch processing in kernel data path, it
> seems there isn't such code for reference.
The checks should be done somewhere in __ovs_nla_copy_action. Which
seems to be done even in your patch by nsh_key_put_from_nlattr
(I didn't get that far during the review last week. The names of
those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to
tell what they do without looking at the call sites - something you
should also consider to improve). That makes it even more wrong: you
appear to check everything twice, first on configuration and then for
every packet.
Jiri
^ permalink raw reply
* Re: [PATCH 1/2] netfilter/xt_hashlimit: new feature/algorithm for xt_hashlimit
From: Pablo Neira Ayuso @ 2017-09-04 10:14 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: kadlec, netfilter-devel, johunt, fw, netdev, pai.vishwain
In-Reply-To: <1503089939-15938-1-git-send-email-vpai@akamai.com>
On Fri, Aug 18, 2017 at 04:58:59PM -0400, Vishwanath Pai wrote:
[...]
> The main difference between the existing algorithm and the new one is
> that the existing algorithm rate-limits the flow whereas the new
> algorithm does not. Instead it *classifies* the flow based on whether
> it is above or below a certain rate. I will demonstrate this with an
> example below. Let us assume this rule:
>
> iptables -A INPUT -m hashlimit --hashlimit-above 10/s -j new_chain
>
> If the packet rate is 15/s, the existing algorithm would ACCEPT 10
> packets every second and send 5 packets to "new_chain".
>
> But with the new algorithm, as long as the rate of 15/s is sustained,
> all packets will continue to match and every packet is sent to new_chain.
Sounds good, applied, thanks.
A couple of questions: Does it really make sense to expose
--hashlimit-rate-interval or are you using 1 second always there? I
always wonder if it makes sense to expose yet another toggle that it's
not clear to me how the user would take advantage from this.
Just curious here.
^ permalink raw reply
* [PATCH v3 net-next 1/2] net: Export tcpv6_prot
From: Ilya Lesokhin @ 2017-09-04 10:14 UTC (permalink / raw)
To: netdev, davem; +Cc: davejwatson, aviadye, Ilya Lesokhin, Boris Pismenny
In-Reply-To: <1504520041-55634-1-git-send-email-ilyal@mellanox.com>
Want to be able to use these in TLS.
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
---
net/ipv6/tcp_ipv6.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 38f76d8..60d0629 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1948,6 +1948,7 @@ struct proto tcpv6_prot = {
#endif
.diag_destroy = tcp_abort,
};
+EXPORT_SYMBOL_GPL(tcpv6_prot);
/* thinking of making this const? Don't.
* early_demux can change based on sysctl.
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 net-next 0/2] Use correct sk->sk_prot for IPV6
From: Ilya Lesokhin @ 2017-09-04 10:13 UTC (permalink / raw)
To: netdev, davem; +Cc: davejwatson, aviadye, Ilya Lesokhin
The tls ulp overrides sk->prot with a new tls specific proto structs.
The tls specific structs were previously based on the ipv4 specific
tcp_prot sturct.
As a result, attaching the tls ulp to an ipv6 tcp socket replaced
some ipv6 callback with the ipv4 equivalents.
This patch adds ipv6 tls proto structs and uses them when
attached to ipv6 sockets.
Changed since v2:
- Dropped patch to fix IPV6_ADDRFORM setsockopt
There was some disagreement about the correct way of fixinig it,
and this series does not depend on it.
Changes since v1:
- TLS now dependes on IPV6
This fixes complication issues when TLS is built-in and IPV6 is a module.
The downside should be small as it is unlikely that there are kernel TLS
users who can't afford to include IPV6 in thier kernel.
- tls_init now checks sk->sk_prot directly
This is somewhat safer then checking indirectly through sk->sk_family
Ilya Lesokhin (2):
net: Export tcpv6_prot
tls: Use correct sk->sk_prot for IPV6
net/ipv6/tcp_ipv6.c | 1 +
net/tls/Kconfig | 1 +
net/tls/tls_main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
3 files changed, 40 insertions(+), 12 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH v3 net-next 2/2] tls: Use correct sk->sk_prot for IPV6
From: Ilya Lesokhin @ 2017-09-04 10:14 UTC (permalink / raw)
To: netdev, davem; +Cc: davejwatson, aviadye, Ilya Lesokhin, Boris Pismenny
In-Reply-To: <1504520041-55634-1-git-send-email-ilyal@mellanox.com>
The tls ulp overrides sk->prot with a new tls specific proto structs.
The tls specific structs were previously based on the ipv4 specific
tcp_prot sturct.
As a result, attaching the tls ulp to an ipv6 tcp socket replaced
some ipv6 callback with the ipv4 equivalents.
This patch adds ipv6 tls proto structs and uses them when
attached to ipv6 sockets.
Fixes: 3c4d7559159b ('tls: kernel TLS support')
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com>
---
net/tls/Kconfig | 1 +
net/tls/tls_main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/net/tls/Kconfig b/net/tls/Kconfig
index eb58303..7e9cf8b 100644
--- a/net/tls/Kconfig
+++ b/net/tls/Kconfig
@@ -7,6 +7,7 @@ config TLS
select CRYPTO
select CRYPTO_AES
select CRYPTO_GCM
+ select IPV6
default n
---help---
Enable kernel support for TLS protocol. This allows symmetric
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 60aff60..33c499e 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -40,13 +40,25 @@
#include <linux/sched/signal.h>
#include <net/tls.h>
+#include <net/transp_v6.h>
MODULE_AUTHOR("Mellanox Technologies");
MODULE_DESCRIPTION("Transport Layer Security Support");
MODULE_LICENSE("Dual BSD/GPL");
-static struct proto tls_base_prot;
-static struct proto tls_sw_prot;
+enum {
+ TLSV4,
+ TLSV6,
+ TLS_NUM_PROTS,
+};
+
+enum {
+ TLS_BASE_TX,
+ TLS_SW_TX,
+ TLS_NUM_CONFIG,
+};
+
+static struct proto tls_prots[TLS_NUM_PROTS][TLS_NUM_CONFIG];
int wait_on_pending_writer(struct sock *sk, long *timeo)
{
@@ -342,6 +354,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
struct tls_context *ctx = tls_get_ctx(sk);
struct proto *prot = NULL;
int rc = 0;
+ int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
if (!optval || (optlen < sizeof(*crypto_info))) {
rc = -EINVAL;
@@ -396,7 +409,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
/* currently SW is default, we will have ethtool in future */
rc = tls_set_sw_offload(sk, ctx);
- prot = &tls_sw_prot;
+ prot = &tls_prots[ip_ver][TLS_SW_TX];
if (rc)
goto err_crypto_info;
@@ -443,6 +456,12 @@ static int tls_init(struct sock *sk)
struct inet_connection_sock *icsk = inet_csk(sk);
struct tls_context *ctx;
int rc = 0;
+ int ip_ver = TLSV4;
+
+ if (sk->sk_prot == &tcpv6_prot)
+ ip_ver = TLSV6;
+ else if (sk->sk_prot != &tcp_prot)
+ return -EINVAL;
/* allocate tls context */
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
@@ -453,7 +472,8 @@ static int tls_init(struct sock *sk)
icsk->icsk_ulp_data = ctx;
ctx->setsockopt = sk->sk_prot->setsockopt;
ctx->getsockopt = sk->sk_prot->getsockopt;
- sk->sk_prot = &tls_base_prot;
+
+ sk->sk_prot = &tls_prots[ip_ver][TLS_BASE_TX];
out:
return rc;
}
@@ -464,16 +484,22 @@ static int tls_init(struct sock *sk)
.init = tls_init,
};
+static void build_protos(struct proto *prot, struct proto *base)
+{
+ prot[TLS_BASE_TX] = *base;
+ prot[TLS_BASE_TX].setsockopt = tls_setsockopt;
+ prot[TLS_BASE_TX].getsockopt = tls_getsockopt;
+
+ prot[TLS_SW_TX] = prot[TLS_BASE_TX];
+ prot[TLS_SW_TX].close = tls_sk_proto_close;
+ prot[TLS_SW_TX].sendmsg = tls_sw_sendmsg;
+ prot[TLS_SW_TX].sendpage = tls_sw_sendpage;
+}
+
static int __init tls_register(void)
{
- tls_base_prot = tcp_prot;
- tls_base_prot.setsockopt = tls_setsockopt;
- tls_base_prot.getsockopt = tls_getsockopt;
-
- tls_sw_prot = tls_base_prot;
- tls_sw_prot.sendmsg = tls_sw_sendmsg;
- tls_sw_prot.sendpage = tls_sw_sendpage;
- tls_sw_prot.close = tls_sk_proto_close;
+ build_protos(tls_prots[TLSV4], &tcp_prot);
+ build_protos(tls_prots[TLSV6], &tcpv6_prot);
tcp_register_ulp(&tcp_tls_ulp_ops);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v3 2/3] dt-binding: net: sfp binding documentation
From: Sergei Shtylyov @ 2017-09-04 9:59 UTC (permalink / raw)
To: Baruch Siach, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, David S. Miller, Russell King
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <01ae7bc45d68322669d18a4feeaeef733ba888d7.1504441874.git.baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
Hello!
On 9/3/2017 3:31 PM, Baruch Siach wrote:
> Add device-tree binding documentation SFP transceivers. Support for SFP
> transceivers has been recently introduced (drivers/net/phy/sfp.c).
>
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
[...]
> diff --git a/Documentation/devicetree/bindings/net/sff,sfp.txt b/Documentation/devicetree/bindings/net/sff,sfp.txt
> new file mode 100644
> index 000000000000..b98ee5945265
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/sff,sfp.txt
> @@ -0,0 +1,78 @@
> +Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
> +Transceiver
> +
> +Required properties:
> +
> +- compatible : must be "sff,sfp"
> +
> +Optional Properties:
> +
> +- i2c-bus : phandle of an I2C bus controller for the SFP two wire serial
> + interface
> +
> +- moddef0-gpios : GPIO phandle and a single specifier of the MOD-DEF0 (AKA
There's always a single specifier following a phandle, tho it may have
different # of cells, no need to spell that out.
> + Mod_ABS) module presence input gpio signal, active (module absent) high
> +
> +- los-gpios : GPIO phandle and a single specifier of the Receiver Loss of
Same here.
> + Signal Indication input gpio signal, active (signal lost) high
> +
> +- tx-fault-gpios : GPIO phandle and a single specifier of the Module
And here.
> + Transmitter Fault input gpio signal, active (fault condition) high
> +
> +- tx-disable-gpios : GPIO phandle and a single specifier of the Transmitter
And here.
> + Disable output gpio signal, active (Tx disable) high
> +
> +- rate-select0-gpios : GPIO phandle and a single specifier of the Rx Signaling
Here too.
> + Rate Select (AKA RS0) output gpio signal, low: low Rx rate, high: high Rx
> + rate
> +
> +- rate-select1-gpios : GPIO phandle and a single specifier of the Tx Signaling
Here ass well.
> + Rate Select (AKA RS1) output gpio signal (SFP+ only), low: low Tx rate,
> + high: high Tx rate
> +
[...]
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [ovs-dev] [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 9:38 UTC (permalink / raw)
To: Hannes Frederic Sowa, Mooney, Sean K
Cc: dev@openvswitch.org, e@erig.me, jbenc@redhat.com,
netdev@vger.kernel.org
In-Reply-To: <87d17boqb1.fsf@stressinduktion.org>
> >> >> Does it makes sense to keep the context headers as part of the flow?
> >> >> What is the reasoning behind it? With mdtype 2 headers this might
> >> >> either not work very well or will increase sw_flow_key size causing
> >> >> slowdowns for all protocols.
> >> > [Mooney, Sean K]
> >> > Having the nsh context headers in the flow is quite useful It would
> >> > allow loadblancing on values stored in the context headers Or other
> >> > use. I belive odl previously used context header 4 to store a Flow id
> >> > so this could potentialy be used with the multipath action to have
> >> ovs
> >> > Choose between several possible next hops in the chain.
> >>
> >> In OVS, masks are a list(!) for matching. How can this work for
> >> different paths that might require different masks? If they can't be
> >> unified you even get exact matches. Thus, for OVS the context should
> >> not be part of the flow.
>
> > [Mooney, Sean K] I'm not sure what you mean about a list as I never
> > made reference to one. md type 1 context headers are 4 mandatory 32
> > bit field.
>
> The semantic of the context fields depend on the path id. Every path can
> have their own interpretation of context fields.
>
> Thus the paths will cetainly have their own masks. I hope I understood
> this part of the standard correctly.
>
> dp only supports installing (approximated) disjoint megaflows and this
> affects performance a lot. Every new mask that gets added to the dp will
> be installed into a list which will be walked sequentially for
> packets. This will kill performance.
>
> I assume that user space slows down, too, depending on the algorithm
> they use generate megaflows.
The primary use case for OVS in SFC/NSH is SFF. In almost all SFF use
cases OVS will not need to match on context headers. The ability of OVS
to match on context headers should not in general slow down the datapath.
When SFC controllers match on parts of the context headers (e.g. in the
final SFF or for load-balancing purposes), we will get additional megaflow
masks. This is a fact of life in OVS and nothing new in NSH. I don't think
this should prevent us from supporting valid use cases in OVS.
The slow-down of the megaflow lookup in the datapath with the number
of masks has been addressed in the user-space datapath with sorting the
mask lists according to hit frequency and maintaining one sorted lists per
ingress port. There is further work in progress to improve on this with a
cuckoo hash to pick the most likely mask first.
It should be possible to apply similar optimization schemes also in the
Kernel datapath.
> > form an ovs context they should be treated the same as the 32 bit register fields.
> > We do not need to necessarily support those in md type 2 as all metadata is optional.
Support for matching on or updating MD2 TLV context headers is FFS in a
future OVS release. We do not foresee to add MD2 TLVs explicitly to the
flow struct.
> > You can also see that context header 1 and 2 are still used in the newer odl netvirt sfc classifier implementation
> > https://github.com/opendaylight/netvirt/blob/ba22f7cf19d8a827d77a3391a7f654344ade43d8/docs/specs/new-sfc-
> classifier.rst#pipeline-changes
> > so for odl existing nsh support to work with md type one we must have the ability to set the nsh context headers
> > and should have the ability to match on them also for load lancing between service function in service function group.
>
> As I said, a separate action sounds much more useful.
I don't think it is a good approach in OVS to introduce custom actions for NSH, e.g. for load sharing based on context header data. The primary tool for load sharing in OpenFlow is the Select Group. OVS already support an extension to the OF 1.5 Group Mod message to specify which fields to hash on. This can be used to hash on the relevant NSH context headers.
BR, Jan
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Hannes Frederic Sowa @ 2017-09-04 9:37 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: Saeed Mahameed, David S. Miller, Linux Netdev List
In-Reply-To: <CALzJLG-KgwRXODJ8BghrYzhdQYtwEM1N1UVwsg+49UiAWbb8uQ@mail.gmail.com>
Hello,
Saeed Mahameed <saeedm@dev.mellanox.co.il> writes:
[...]
> On Sat, Sep 2, 2017 at 6:32 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Sorry, I think I am still confused.
>>
>> I just want to make sure that you don't use the first nibble after the
>> mpls bottom of stack label in any way as an indicator if that is an IPv4
>> or IPv6 packet by default. It can be anything. The forward equivalence
>> class tells the stack which protocol you see.
>>
>> If you match on the first nibble behind the MPLS bottom of stack label
>> the '4' or '6' respectively could be part of a MAC address with its
>> first nibble being 4 or 6, because the particular pseudowire is EoMPLS
>> and uses no control world.
>>
>> I wanted to mention it, because with addition of e.g. VPLS this could
>> cause problems down the road and should at least be controllable? It is
>> probably better to use Entropy Labels in future.
>>
>
> Hi Hannes,
>
> I see your concern now, but still it has nothing to do with the
> driver, the whole change is only to simplify driver code to not push
> full blown matching steering rules into the HW, and simply replace it
> with a one bit command.
Sorry, I very much got the impression from the cover letter plus the
descriptions of the patches.
> Regarding your concern, I will need to check with the HW guys and
> review the processing algorithm that identifies IP packets over MPLs,
> and will get back to you.
>
> if there is really a problem, then yes, we might need to make it
> controllable ..
Thanks for looking into this. I do think it can be added as a feature
but I very much think such logic should be disabled by default.
Thanks,
Hannes
^ permalink raw reply
* Re: netdev watchdog enp1s0 (tg3): transmit queue 0 timed out
From: Siva Reddy Kallam @ 2017-09-04 9:09 UTC (permalink / raw)
To: Frans van Berckel; +Cc: Michael Chan, linux-netdev
On Fri, Sep 1, 2017 at 12:13 PM, Frans van Berckel <fberckel@xs4all.nl> wrote:
> Dear Michael and Siva,
>
> On Thu, 2017-08-31 at 23:36 -0700, Michael Chan wrote:
>> On Thu, Aug 31, 2017 at 11:10 PM, Frans van Berckel
>> <fberckel@xs4all.nl> wrote:
>> >
>> > <snap> a long list of likely the same type of error codes.
>> >
>>
>> Please post the entire register dump.
>
> [ 237.169194] tg3 0000:01:00.0 enp1s0: Link is up at 1000 Mbps, full
> duplex
> [ 237.169335] tg3 0000:01:00.0 enp1s0: Flow control is on for TX and
> on for RX
> [ 237.169375] IPv6: ADDRCONF(NETDEV_CHANGE): enp1s0: link becomes
> ready
> [ 243.683910] tg3 0000:01:00.0 enp1s0: DMA Status error. Resetting
> chip.
> [ 243.759610] hrtimer: interrupt took 9464192 ns
> [ 245.317566] tg3 0000:01:00.0 enp1s0: 0x00000000: 0x165914e4,
> 0x00100406, 0x02000021, 0x00000010
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000010: 0xefcf0004,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000020: 0x00000000,
> 0x00000000, 0x00000000, 0x01eb1028
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000030: 0x00000000,
> 0x00000048, 0x00000000, 0x00000105
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000040: 0x00000000,
> 0x00000000, 0xc0025001, 0x64002000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000050: 0x00fc5803,
> 0x80818283, 0x0087d005, 0xfee0300c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000060: 0x00000000,
> 0x00004122, 0x42010298, 0x76180000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000070: 0x000010f2,
> 0x000000a0, 0x00000000, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000080: 0x165914e4,
> 0x3c1d0002, 0x04130034, 0x3c085082
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000090: 0x01009509,
> 0x00000000, 0x00000000, 0x000000ce
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000000a0: 0x00000000,
> 0x00000006, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000000b0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000000c0: 0x00000000,
> 0x00000000, 0x0000000e, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000000d0: 0x00010010,
> 0x00000fa0, 0x00122104, 0x00036c11
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000000e0: 0x10110000,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000100: 0x13c10001,
> 0x00014000, 0x0011c000, 0x000e3010
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000110: 0x00000000,
> 0x000011c1, 0x000000a0, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000130: 0x00000000,
> 0x00000000, 0x00000000, 0x16010002
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000150: 0x800000ff,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000160: 0x16c10003,
> 0xfe3b2547, 0x001ec9ff, 0x00010004
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000170: 0x00000000,
> 0x0007810e, 0x00000001, 0x2c0c3c0e
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000180: 0x3f062304,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000200: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000210: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000220: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000230: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000240: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000250: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000260: 0x00000000,
> 0x00000001, 0x00000000, 0x000000ce
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000270: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000280: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000290: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000002a0: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000002b0: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000002c0: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000002d0: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000002e0: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000002f0: 0x00000000,
> 0x00000006, 0x00000000, 0x00000006
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000300: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000310: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000320: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000330: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000340: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000350: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000360: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000370: 0x00000000,
> 0x0000000f, 0x00000000, 0x0000000f
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000380: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000390: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000003a0: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000003b0: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000003c0: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000003d0: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000003e0: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x000003f0: 0x00000000,
> 0x0000000c, 0x00000000, 0x0000000c
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000400: 0x00e04808,
> 0x00400000, 0x00001000, 0x00000880
> [ 245.476295] tg3 0000:01:00.0 enp1s0: 0x00000410: 0x0000001e,
> 0xc93b2547, 0x0000001e, 0xc93b2547
> [ 245.502584] tg3 0000:01:00.0 enp1s0: 0x00000420: 0x0000001e,
> 0xc93b2547, 0x0000001e, 0xc93b2547
> [ 245.504134] tg3 0000:01:00.0 enp1s0: 0x00000430: 0x00000000,
> 0x00000000, 0x000002f8, 0x000005f2
> [ 245.505060] tg3 0000:01:00.0 enp1s0: 0x00000440: 0x00000000,
> 0x00000000, 0x00000000, 0x08310301
> [ 245.506073] tg3 0000:01:00.0 enp1s0: 0x00000450: 0x00000001,
> 0x000c0000, 0x00000000, 0x00000012
> [ 245.508062] tg3 0000:01:00.0 enp1s0: 0x00000460: 0x00000008,
> 0x00002620, 0x00000006, 0x00000000
> [ 245.509057] tg3 0000:01:00.0 enp1s0: 0x00000470: 0xa0000080,
> 0x00000000, 0x00000000, 0x50000000
> [ 245.510120] tg3 0000:01:00.0 enp1s0: 0x00000480: 0x42000000,
> 0x7fffffff, 0x06000004, 0x7fffffff
> [ 245.512072] tg3 0000:01:00.0 enp1s0: 0x00000500: 0x00000008,
> 0x00000002, 0x00000000, 0x00000000
> [ 245.513052] tg3 0000:01:00.0 enp1s0: 0x00000590: 0x01e00000,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.514132] tg3 0000:01:00.0 enp1s0: 0x00000600: 0xffffffff,
> 0x00f80011, 0x00000000, 0x00001f04
> [ 245.516108] tg3 0000:01:00.0 enp1s0: 0x00000610: 0xffffffff,
> 0x00000000, 0x07c00004, 0x36ecf800
> [ 245.517074] tg3 0000:01:00.0 enp1s0: 0x00000620: 0x00000040,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.518121] tg3 0000:01:00.0 enp1s0: 0x00000800: 0x00000000,
> 0xffffffff, 0x00000000, 0x00000000
> [ 245.520060] tg3 0000:01:00.0 enp1s0: 0x00000810: 0x00000000,
> 0xffffffff, 0x00000000, 0x00000000
> [ 245.521077] tg3 0000:01:00.0 enp1s0: 0x00000820: 0x00000000,
> 0x00000000, 0xffffffff, 0x00000000
> [ 245.523047] tg3 0000:01:00.0 enp1s0: 0x00000830: 0x00000000,
> 0xffffffff, 0xffffffff, 0xffffffff
> [ 245.524073] tg3 0000:01:00.0 enp1s0: 0x00000840: 0xffffffff,
> 0xffffffff, 0xffffffff, 0xffffffff
> [ 245.525063] tg3 0000:01:00.0 enp1s0: 0x00000850: 0xffffffff,
> 0xffffffff, 0xffffffff, 0xffffffff
> [ 245.527048] tg3 0000:01:00.0 enp1s0: 0x00000860: 0xffffffff,
> 0xffffffff, 0xffffffff, 0x00000000
> [ 245.528073] tg3 0000:01:00.0 enp1s0: 0x00000880: 0x00000040,
> 0x000001a8, 0x00000000, 0x00000001
> [ 245.529070] tg3 0000:01:00.0 enp1s0: 0x000008f0: 0x00000001,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.530174] tg3 0000:01:00.0 enp1s0: 0x00000c00: 0x0000000a,
> 0x00000000, 0x00000003, 0x00000001
> [ 245.532064] tg3 0000:01:00.0 enp1s0: 0x00000c10: 0x00000000,
> 0x00000000, 0x00000000, 0x004d0000
> [ 245.533072] tg3 0000:01:00.0 enp1s0: 0x00000c80: 0x0000000c,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.534079] tg3 0000:01:00.0 enp1s0: 0x00000ce0: 0x790cb002,
> 0x00000000, 0x0000004d, 0x00041028
> [ 245.536073] tg3 0000:01:00.0 enp1s0: 0x00000cf0: 0x00000000,
> 0x5000000c, 0x00000000, 0x00000000
> [ 245.537070] tg3 0000:01:00.0 enp1s0: 0x00001000: 0x00000002,
> 0x00000000, 0xa0000618, 0x00000000
> [ 245.539096] tg3 0000:01:00.0 enp1s0: 0x00001010: 0x000c00c0,
> 0x00000618, 0x00000000, 0x00000000
> [ 245.540065] tg3 0000:01:00.0 enp1s0: 0x00001400: 0x00000006,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.542048] tg3 0000:01:00.0 enp1s0: 0x00001440: 0x0000000c,
> 0x0000000c, 0x0000000c, 0x0000000c
> [ 245.543056] tg3 0000:01:00.0 enp1s0: 0x00001450: 0x0000000c,
> 0x0000000c, 0x0000000c, 0x0000000c
> [ 245.544064] tg3 0000:01:00.0 enp1s0: 0x00001460: 0x0000000c,
> 0x0000000c, 0x0000000c, 0x0000000c
> [ 245.546056] tg3 0000:01:00.0 enp1s0: 0x00001470: 0x0000000c,
> 0x0000000c, 0x0000000c, 0x0000000c
> [ 245.548047] tg3 0000:01:00.0 enp1s0: 0x00001480: 0x00000001,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.549060] tg3 0000:01:00.0 enp1s0: 0x00001800: 0x00000016,
> 0x00000000, 0x0000000d, 0x00000000
> [ 245.551128] tg3 0000:01:00.0 enp1s0: 0x00001840: 0x00000000,
> 0x00000000, 0x00000710, 0x00000010
> [ 245.552065] tg3 0000:01:00.0 enp1s0: 0x00001850: 0x752260d0,
> 0x00000000, 0x000040d0, 0x000d000e
> [ 245.554053] tg3 0000:01:00.0 enp1s0: 0x00001860: 0x01000100,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.555073] tg3 0000:01:00.0 enp1s0: 0x00001c00: 0x00000002,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.557059] tg3 0000:01:00.0 enp1s0: 0x00002000: 0x00000002,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.559055] tg3 0000:01:00.0 enp1s0: 0x00002010: 0x00000181,
> 0x00000001, 0x00790807, 0x00000000
> [ 245.560071] tg3 0000:01:00.0 enp1s0: 0x00002100: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.562058] tg3 0000:01:00.0 enp1s0: 0x00002110: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.564065] tg3 0000:01:00.0 enp1s0: 0x00002120: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.566127] tg3 0000:01:00.0 enp1s0: 0x00002130: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.567066] tg3 0000:01:00.0 enp1s0: 0x00002140: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.569055] tg3 0000:01:00.0 enp1s0: 0x00002150: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.571063] tg3 0000:01:00.0 enp1s0: 0x00002160: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.573128] tg3 0000:01:00.0 enp1s0: 0x00002170: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.574065] tg3 0000:01:00.0 enp1s0: 0x00002180: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.576057] tg3 0000:01:00.0 enp1s0: 0x00002190: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.578063] tg3 0000:01:00.0 enp1s0: 0x000021a0: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.580060] tg3 0000:01:00.0 enp1s0: 0x000021b0: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.582128] tg3 0000:01:00.0 enp1s0: 0x000021c0: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.583057] tg3 0000:01:00.0 enp1s0: 0x000021d0: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.585055] tg3 0000:01:00.0 enp1s0: 0x000021e0: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.587060] tg3 0000:01:00.0 enp1s0: 0x000021f0: 0x000c6c36,
> 0x000c6c36, 0x00000000, 0x00000000
> [ 245.589126] tg3 0000:01:00.0 enp1s0: 0x00002200: 0x00000009,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.590057] tg3 0000:01:00.0 enp1s0: 0x00002400: 0x00000012,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.592058] tg3 0000:01:00.0 enp1s0: 0x00002450: 0x00000000,
> 0x74b1c000, 0x02000000, 0x00006000
> [ 245.594128] tg3 0000:01:00.0 enp1s0: 0x00002470: 0x00000000,
> 0x00000007, 0x00000000, 0x00000000
> [ 245.595057] tg3 0000:01:00.0 enp1s0: 0x000024c0: 0x08000001,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.597062] tg3 0000:01:00.0 enp1s0: 0x00002800: 0x00000006,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.599128] tg3 0000:01:00.0 enp1s0: 0x00002c00: 0x00000006,
> 0x00000000, 0x00000000, 0x0000007f
> [ 245.600064] tg3 0000:01:00.0 enp1s0: 0x00002c10: 0x00000000,
> 0x00000000, 0x00000008, 0x00000000
> [ 245.602066] tg3 0000:01:00.0 enp1s0: 0x00002c20: 0x00000001,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.603057] tg3 0000:01:00.0 enp1s0: 0x00003000: 0x00000006,
> 0x00000000, 0x00000000, 0x0000007f
> [ 245.605064] tg3 0000:01:00.0 enp1s0: 0x00003c00: 0x00000302,
> 0x00000000, 0x00000014, 0x00000048
> [ 245.606056] tg3 0000:01:00.0 enp1s0: 0x00003c10: 0x00000005,
> 0x00000035, 0x00000000, 0x00000000
> [ 245.608059] tg3 0000:01:00.0 enp1s0: 0x00003c20: 0x00000005,
> 0x00000005, 0x00000000, 0x00000000
> [ 245.610128] tg3 0000:01:00.0 enp1s0: 0x00003c30: 0x00000000,
> 0x00000000, 0x00000000, 0x73e53000
> [ 245.611065] tg3 0000:01:00.0 enp1s0: 0x00003c40: 0x00000000,
> 0x00000b00, 0x00000000, 0x00000000
> [ 245.613128] tg3 0000:01:00.0 enp1s0: 0x00003c50: 0x00000000,
> 0x00000007, 0x00000000, 0x00000000
> [ 245.614060] tg3 0000:01:00.0 enp1s0: 0x00003c80: 0x00000007,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.616070] tg3 0000:01:00.0 enp1s0: 0x00003cc0: 0x0000000c,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.617061] tg3 0000:01:00.0 enp1s0: 0x00004000: 0x00000002,
> 0x00000000, 0x001ebffc, 0x001635cf
> [ 245.619063] tg3 0000:01:00.0 enp1s0: 0x00004010: 0x004186a0,
> 0x00236012, 0x00800440, 0x01001862
> [ 245.620062] tg3 0000:01:00.0 enp1s0: 0x00004020: 0x0020eb11,
> 0x00000000, 0x00000010, 0x00000000
> [ 245.622067] tg3 0000:01:00.0 enp1s0: 0x00004030: 0x00000010,
> 0x00000030, 0x00000000, 0x00000000
> [ 245.623060] tg3 0000:01:00.0 enp1s0: 0x00004040: 0x00000000,
> 0x00000000, 0x01081620, 0x00000000
> [ 245.625060] tg3 0000:01:00.0 enp1s0: 0x00004050: 0x00000000,
> 0x00000000, 0x00236010, 0x00413002
> [ 245.626060] tg3 0000:01:00.0 enp1s0: 0x00004060: 0x00419000,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.628066] tg3 0000:01:00.0 enp1s0: 0x00004400: 0x00000006,
> 0x00000000, 0x00011080, 0x0000df80
> [ 245.630096] tg3 0000:01:00.0 enp1s0: 0x00004410: 0x00000000,
> 0x00000010, 0x00000060, 0x00000000
> [ 245.631066] tg3 0000:01:00.0 enp1s0: 0x00004420: 0x0000003d,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.633129] tg3 0000:01:00.0 enp1s0: 0x00004440: 0x00000000,
> 0x00000000, 0x00000000, 0x0180601a
> [ 245.634062] tg3 0000:01:00.0 enp1s0: 0x00004450: 0x000103be,
> 0x00160017, 0x00000000, 0x00000000
> [ 245.636063] tg3 0000:01:00.0 enp1s0: 0x00004800: 0x080303fe,
> 0x00000010, 0x01000019, 0x00000020
> [ 245.637054] tg3 0000:01:00.0 enp1s0: 0x00004810: 0x00000011,
> 0x00000004, 0x00000000, 0x00000004
> [ 245.639064] tg3 0000:01:00.0 enp1s0: 0x00004820: 0x00100019,
> 0x00000000, 0xa0a40010, 0x752260c0
> [ 245.641128] tg3 0000:01:00.0 enp1s0: 0x00004830: 0x00000010,
> 0x000000e4, 0x000000e4, 0x00000000
> [ 245.642055] tg3 0000:01:00.0 enp1s0: 0x00004840: 0x0000000c,
> 0x0000000c, 0x030e2200, 0x192a3f66
> [ 245.644065] tg3 0000:01:00.0 enp1s0: 0x00004850: 0xffff7da7,
> 0x83b1004d, 0x13181318, 0x00000000
> [ 245.646128] tg3 0000:01:00.0 enp1s0: 0x00004c00: 0x000003fe,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.647075] tg3 0000:01:00.0 enp1s0: 0x00004c10: 0x00000000,
> 0x00000000, 0x00000006, 0x00000000
> [ 245.649061] tg3 0000:01:00.0 enp1s0: 0x00004c20: 0x00000000,
> 0x00000000, 0x00000000, 0x00000006
> [ 245.651079] tg3 0000:01:00.0 enp1s0: 0x00004c30: 0x00000000,
> 0x00000000, 0x00000056, 0x00000056
> [ 245.653047] tg3 0000:01:00.0 enp1s0: 0x00004c40: 0x00000000,
> 0x73e53000, 0x00010020, 0x00000020
> [ 245.654055] tg3 0000:01:00.0 enp1s0: 0x00005000: 0x00009800,
> 0x80004000, 0x00000000, 0x00000000
> [ 245.656071] tg3 0000:01:00.0 enp1s0: 0x00005010: 0x00000000,
> 0x00000000, 0x00000000, 0x0001081c
> [ 245.658101] tg3 0000:01:00.0 enp1s0: 0x00005020: 0x8fb20018,
> 0x00000000, 0x00000000, 0x40000020
> [ 245.659056] tg3 0000:01:00.0 enp1s0: 0x00005030: 0x00000000,
> 0x0000001d, 0x00000000, 0x00000000
> [ 245.661063] tg3 0000:01:00.0 enp1s0: 0x00005040: 0x00000000,
> 0x00000000, 0x00010dd2, 0x00000000
> [ 245.663067] tg3 0000:01:00.0 enp1s0: 0x00005200: 0x00000000,
> 0xb49a89ab, 0xfffeffff, 0x00000000
> [ 245.665055] tg3 0000:01:00.0 enp1s0: 0x00005210: 0x0001ff90,
> 0x00010820, 0x00000000, 0x0001ff90
> [ 245.667060] tg3 0000:01:00.0 enp1s0: 0x00005220: 0xb49a89ab,
> 0x00000000, 0xb49a89ab, 0x00000000
> [ 245.669071] tg3 0000:01:00.0 enp1s0: 0x00005230: 0x00000000,
> 0x00000000, 0x0001ff90, 0x00000000
> [ 245.671066] tg3 0000:01:00.0 enp1s0: 0x00005240: 0x0001ff90,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.673101] tg3 0000:01:00.0 enp1s0: 0x00005250: 0x00000000,
> 0x0001ff90, 0x00000000, 0x0001ff90
> [ 245.675092] tg3 0000:01:00.0 enp1s0: 0x00005260: 0x00000000,
> 0x0001ff90, 0x00000000, 0xb49a89ab
> [ 245.677079] tg3 0000:01:00.0 enp1s0: 0x00005270: 0x00000000,
> 0x0001ff90, 0x00010820, 0x00000000
> [ 245.679067] tg3 0000:01:00.0 enp1s0: 0x00005800: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.681070] tg3 0000:01:00.0 enp1s0: 0x00005810: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.683076] tg3 0000:01:00.0 enp1s0: 0x00005820: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.685072] tg3 0000:01:00.0 enp1s0: 0x00005830: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.687070] tg3 0000:01:00.0 enp1s0: 0x00005840: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.689073] tg3 0000:01:00.0 enp1s0: 0x00005850: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.691074] tg3 0000:01:00.0 enp1s0: 0x00005860: 0x00000000,
> 0x00000001, 0x00000000, 0x000000ce
> [ 245.693070] tg3 0000:01:00.0 enp1s0: 0x00005870: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.695068] tg3 0000:01:00.0 enp1s0: 0x00005880: 0x00000000,
> 0x00000006, 0x00000000, 0x00000001
> [ 245.697060] tg3 0000:01:00.0 enp1s0: 0x00005890: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.699057] tg3 0000:01:00.0 enp1s0: 0x000058a0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.701053] tg3 0000:01:00.0 enp1s0: 0x000058b0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.703058] tg3 0000:01:00.0 enp1s0: 0x000058c0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.705128] tg3 0000:01:00.0 enp1s0: 0x000058d0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.707128] tg3 0000:01:00.0 enp1s0: 0x000058e0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.708075] tg3 0000:01:00.0 enp1s0: 0x000058f0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.710063] tg3 0000:01:00.0 enp1s0: 0x00005900: 0x00000000,
> 0x0000000f, 0x00000000, 0x00000001
> [ 245.712060] tg3 0000:01:00.0 enp1s0: 0x00005910: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.714068] tg3 0000:01:00.0 enp1s0: 0x00005920: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.716073] tg3 0000:01:00.0 enp1s0: 0x00005930: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.718052] tg3 0000:01:00.0 enp1s0: 0x00005940: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.720081] tg3 0000:01:00.0 enp1s0: 0x00005950: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.722047] tg3 0000:01:00.0 enp1s0: 0x00005960: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.723070] tg3 0000:01:00.0 enp1s0: 0x00005970: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.725063] tg3 0000:01:00.0 enp1s0: 0x00005980: 0x00000000,
> 0x0000000c, 0x00000000, 0x00000001
> [ 245.727055] tg3 0000:01:00.0 enp1s0: 0x00005990: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.729047] tg3 0000:01:00.0 enp1s0: 0x000059a0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.730074] tg3 0000:01:00.0 enp1s0: 0x000059b0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.732060] tg3 0000:01:00.0 enp1s0: 0x000059c0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.734095] tg3 0000:01:00.0 enp1s0: 0x000059d0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.735079] tg3 0000:01:00.0 enp1s0: 0x000059e0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.737056] tg3 0000:01:00.0 enp1s0: 0x000059f0: 0x00000000,
> 0x00000001, 0x00000000, 0x00000001
> [ 245.738058] tg3 0000:01:00.0 enp1s0: 0x00005a00: 0x00012001,
> 0x00000000, 0x00010000, 0x00000000
> [ 245.740054] tg3 0000:01:00.0 enp1s0: 0x00006000: 0x00000002,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.742127] tg3 0000:01:00.0 enp1s0: 0x00006800: 0x04130034,
> 0x3c085082, 0x01009509, 0x0c502cdc
> [ 245.743051] tg3 0000:01:00.0 enp1s0: 0x00006810: 0x81120000,
> 0xffffffff, 0x00000000, 0x00000000
> [ 245.745067] tg3 0000:01:00.0 enp1s0: 0x00006830: 0xfffc3ccf,
> 0xfffc0fff, 0x00000000, 0x00000000
> [ 245.746066] tg3 0000:01:00.0 enp1s0: 0x00006840: 0x00000024,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.748061] tg3 0000:01:00.0 enp1s0: 0x00006c00: 0x00000040,
> 0x00000000, 0x00000000, 0x00000000
> [ 245.749074] tg3 0000:01:00.0 enp1s0: 0x00006c40: 0x00000000,
> 0x000f0000, 0x00000000, 0x000000f7
> [ 245.751054] tg3 0000:01:00.0 enp1s0: 0x00006c50: 0x00000000,
> 0x00000000, 0x000000f7, 0x00000000
> [ 245.753096] tg3 0000:01:00.0 enp1s0: 0x00007000: 0x00000188,
> 0x00000000, 0x00000000, 0x000000c0
> [ 245.754055] tg3 0000:01:00.0 enp1s0: 0x00007010: 0x0a000064,
> 0x02008273, 0x00570081, 0x68848353
> [ 245.756109] tg3 0000:01:00.0 enp1s0: 0x00007020: 0x00000000,
> 0x00000000, 0xaf000400, 0x00000000
> [ 245.757052] tg3 0000:01:00.0 enp1s0: 0x00007400: 0x00000000,
> 0x000000aa, 0x00000000, 0x00000000
> [ 245.759057] tg3 0000:01:00.0 enp1s0: 0x00007800: 0x00000000,
> 0x00000000, 0x00000001, 0x00000000
> [ 245.760055] tg3 0000:01:00.0 enp1s0: 0x00007810: 0x00000000,
> 0x00000060, 0x0000000d, 0x00000000
> [ 245.762065] tg3 0000:01:00.0 enp1s0: 0: Host status block
> [00000005:00000016:(0000:0007:0000):(0007:000c)]
> [ 245.764128] tg3 0000:01:00.0 enp1s0: 0: NAPI info
> [00000015:00000015:(000f:000c:01ff):0006:(00ce:0000:0000:0000)]
> [ 245.867391] tg3 0000:01:00.0: tg3_stop_block timed out, ofs=2c00
> enable_bit=2
> [ 245.971098] tg3 0000:01:00.0: tg3_stop_block timed out, ofs=4800
> enable_bit=2
> [ 245.993343] tg3 0000:01:00.0 enp1s0: Link is down
> [ 249.731158] tg3 0000:01:00.0 enp1s0: Link is up at 1000 Mbps, full
> duplex
> [ 249.731336] tg3 0000:01:00.0 enp1s0: Flow control is on for TX and
> on for RX
> [ 254.944022] ------------[ cut here ]------------
> [ 254.945010] WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:316
> dev_watchdog+0x221/0x230
> [ 254.945010] NETDEV WATCHDOG: enp1s0 (tg3): transmit queue 0 timed
> out
> [ 254.945010] Modules linked in: xt_CHECKSUM ipt_MASQUERADE
> nf_nat_masquerade_ipv4 tun fuse nf_conntrack_netbios_ns
> nf_conntrack_broadcast xt_CT ip6t_rpfilter ip6t_REJECT nf_reject_ipv6
> xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge
> ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6
> ip6table_mangle ip6table_raw ip6table_security iptable_nat
> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack
> libcrc32c iptable_mangle iptable_raw iptable_security ebtable_filter
> ebtables ip6table_filter ip6_tables powernow_k8 amd64_edac_mod
> edac_mce_amd edac_core kvm_amd kvm irqbypass amdkfd amd_iommu_v2 dcdbas
> radeon k8temp ipmi_ssif i2c_algo_bit ttm drm_kms_helper drm ipmi_si
> ipmi_devintf tpm_tis ipmi_msghandler tpm_tis_core tpm i2c_piix4 shpchp
> nls_utf8 isofs squashfs ata_generic
> [ 254.945010] pata_acpi uas usb_storage 8021q garp stp llc mrp
> serio_raw tg3 sata_sil24 ptp pps_core pata_serverworks sunrpc
> scsi_transport_iscsi loop
> [ 254.945010] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.8-
> 300.fc26.x86_64 #1
> [ 254.945010] Hardware name: Dell Inc. PowerEdge SC1435/0YR707, BIOS
> 2.2.5 03/21/2008
> [ 254.945010] Call Trace:
> [ 254.945010] <IRQ>
> [ 254.945010] dump_stack+0x63/0x84
> [ 254.945010] __warn+0xcb/0xf0
> [ 254.945010] warn_slowpath_fmt+0x5a/0x80
> [ 254.945010] dev_watchdog+0x221/0x230
> [ 254.945010] ? qdisc_rcu_free+0x50/0x50
> [ 254.945010] call_timer_fn+0x33/0x130
> [ 254.945010] run_timer_softirq+0x3ee/0x440
> [ 254.945010] ? ktime_get+0x40/0xb0
> [ 254.945010] ? lapic_next_event+0x1d/0x30
> [ 254.945010] __do_softirq+0xea/0x2e3
> [ 254.945010] irq_exit+0xfb/0x100
> [ 254.945010] smp_apic_timer_interrupt+0x3d/0x50
> [ 254.945010] apic_timer_interrupt+0x93/0xa0
> [ 254.945010] RIP: 0010:native_safe_halt+0x6/0x10
> [ 254.945010] RSP: 0018:ffffbabf4038be60 EFLAGS: 00000246 ORIG_RAX:
> ffffffffffffff10
> [ 254.945010] RAX: 6874754100002d40 RBX: ffff926d39d04880 RCX:
> 0000000000000000
> [ 254.945010] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> 0000000000000000
> [ 254.945010] RBP: ffffbabf4038be60 R08: ffff926d3d052ae0 R09:
> 0000000000000000
> [ 254.945010] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000001
> [ 254.945010] R13: ffff926d39d04880 R14: 0000000000000000 R15:
> 0000000000000000
> [ 254.945010] </IRQ>
> [ 254.945010] default_idle+0x20/0xe0
> [ 254.945010] amd_e400_idle+0x3f/0x50
> [ 254.945010] arch_cpu_idle+0xf/0x20
> [ 254.945010] default_idle_call+0x23/0x30
> [ 254.945010] do_idle+0x170/0x200
> [ 254.945010] cpu_startup_entry+0x71/0x80
> [ 254.945010] start_secondary+0x154/0x190
> [ 254.945010] start_cpu+0x14/0x14
> [ 255.080137] ---[ end trace 25a535e6d8610c90 ]---
> [ 255.080137] tg3 0000:01:00.0 enp1s0: transmit timed out, resetting
Can you please share below commands output as well?
lspci -v
lspci -t
^ permalink raw reply
* [PATCH net-next v7] net: stmmac: Delete dead code for MDIO registration
From: Romain Perier @ 2017-09-04 8:41 UTC (permalink / raw)
To: Giuseppe Cavallaro, Alexandre Torgue, Andrew Lunn,
Florian Fainelli
Cc: David S. Miller, netdev, linux-kernel, Romain Perier
This code is no longer used, the logging function was changed by commit
fbca164776e4 ("net: stmmac: Use the right logging function in stmmac_mdio_register").
It was previously showing information about the type of the IRQ, if it's
polled, ignored or a normal interrupt. As we don't want information loss,
I have moved this code to phy_attached_print().
Fixes: fbca164776e4 ("net: stmmac: Use the right logging function in stmmac_mdio_register")
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
Hello,
This is the continuity of "[PATCH v2 0/2] net: stmmac: phy logging fixes",
see https://lkml.org/lkml/2017/8/21/288
Changes in v7:
- Rebased onto net-next
Changes in v6:
- Rebased onto net-next
Changes in v5:
- Don't truncater commit fbca164776e4 in the message of *this* commit
- Fixed typo
Changes in v4:
- Don't truncate the commit subject for "Fixes"
- Fixed invalid sizeof() used with snprintf
- Added "net-next" prefix to the title of the commit
Changes in v3:
- Removed patch 2/2: "net: phy: Don't use drv when it is NULL in phy_attached_print",
fixed upstream by fcd03e362b1c
("net: phy: Deal with unbound PHY driver in phy_attached_print()")
- Moved this code to phy_attached_print(), so we have more informations
about the IRQ (poll, ignore or normal irq) and no information are loss.
- Re-worded commit message
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 ----------------
drivers/net/phy/phy_device.c | 22 +++++++++++++++++++---
2 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 72ec711fcba2..f5f37bfa1d58 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -248,9 +248,6 @@ int stmmac_mdio_register(struct net_device *ndev)
found = 0;
for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
struct phy_device *phydev = mdiobus_get_phy(new_bus, addr);
- int act = 0;
- char irq_num[4];
- char *irq_str;
if (!phydev)
continue;
@@ -273,19 +270,6 @@ int stmmac_mdio_register(struct net_device *ndev)
if (priv->plat->phy_addr == -1)
priv->plat->phy_addr = addr;
- act = (priv->plat->phy_addr == addr);
- switch (phydev->irq) {
- case PHY_POLL:
- irq_str = "POLL";
- break;
- case PHY_IGNORE_INTERRUPT:
- irq_str = "IGNORE";
- break;
- default:
- sprintf(irq_num, "%d", phydev->irq);
- irq_str = irq_num;
- break;
- }
phy_attached_info(phydev);
found = 1;
}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 810f6fd2f639..8cf0c5901f95 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -874,21 +874,37 @@ void phy_attached_info(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_attached_info);
-#define ATTACHED_FMT "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)"
+#define ATTACHED_FMT "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%s)"
void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
{
const char *drv_name = phydev->drv ? phydev->drv->name : "unbound";
+ char *irq_str;
+ char irq_num[4];
+
+ switch(phydev->irq) {
+ case PHY_POLL:
+ irq_str = "POLL";
+ break;
+ case PHY_IGNORE_INTERRUPT:
+ irq_str = "IGNORE";
+ break;
+ default:
+ snprintf(irq_num, sizeof(irq_num), "%d", phydev->irq);
+ irq_str = irq_num;
+ break;
+ }
+
if (!fmt) {
dev_info(&phydev->mdio.dev, ATTACHED_FMT "\n",
drv_name, phydev_name(phydev),
- phydev->irq);
+ irq_str);
} else {
va_list ap;
dev_info(&phydev->mdio.dev, ATTACHED_FMT,
drv_name, phydev_name(phydev),
- phydev->irq);
+ irq_str);
va_start(ap, fmt);
vprintk(fmt, ap);
--
2.11.0
^ permalink raw reply related
* [PATCH v3 2/2] ip6_tunnel: fix ip6 tunnel lookup in collect_md mode
From: Haishuang Yan @ 2017-09-04 8:36 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI
Cc: netdev, linux-kernel, Haishuang Yan, Alexei Starovoitov
In-Reply-To: <1504514174-14958-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
In collect_md mode, if the tun dev is down, it still can call
__ip6_tnl_rcv to receive on packets, and the rx statistics increase
improperly.
Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
Cc: Alexei Starovoitov <ast@fb.com>
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
Change since v3:
* Increment rx_dropped if tunnel device is not up, suggested by
Pravin B Shelar
* Fix wrong recipient address
---
net/ipv6/ip6_tunnel.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 10a693a..e91d3b6 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -171,8 +171,11 @@ static struct net_device_stats *ip6_get_stats(struct net_device *dev)
}
t = rcu_dereference(ip6n->collect_md_tun);
- if (t)
- return t;
+ if (t) {
+ if (t->dev->flags & IFF_UP)
+ return t;
+ t->dev->stats.rx_dropped++;
+ }
t = rcu_dereference(ip6n->tnls_wc[0]);
if (t && (t->dev->flags & IFF_UP))
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 1/2] ip_tunnel: fix ip tunnel lookup in collect_md mode
From: Haishuang Yan @ 2017-09-04 8:36 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI
Cc: netdev, linux-kernel, Haishuang Yan, Pravin B Shelar
In collect_md mode, if the tun dev is down, it still can call
ip_tunnel_rcv to receive on packets, and the rx statistics increase
improperly.
Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
Cc: Pravin B Shelar <pshelar@nicira.com>
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
Change since v3:
* Increment rx_dropped if tunnel device is not up, suggested by
Pravin B Shelar
* Fix wrong recipient addresss
---
net/ipv4/ip_tunnel.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e1856bf..5b2e542 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -176,8 +176,11 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
return cand;
t = rcu_dereference(itn->collect_md_tun);
- if (t)
- return t;
+ if (t) {
+ if (t->dev->flags & IFF_UP)
+ return t;
+ t->dev->stats.rx_dropped++;
+ }
if (itn->fb_tunnel_dev && itn->fb_tunnel_dev->flags & IFF_UP)
return netdev_priv(itn->fb_tunnel_dev);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] mm/vmstats: add counters for the page frag cache
From: Konstantin Khlebnikov @ 2017-09-04 8:30 UTC (permalink / raw)
To: Kyeongdon Kim, akpm, sfr
Cc: ying.huang, vbabka, hannes, xieyisheng1, luto, shli, mhocko,
mgorman, hillf.zj, kemi.wang, rientjes, bigeasy, iamjoonsoo.kim,
bongkyu.kim, linux-kernel, linux-mm, netdev
In-Reply-To: <19156a13-6153-f570-317b-7b80505347e7@lge.com>
On 04.09.2017 04:35, Kyeongdon Kim wrote:
> Thanks for your reply,
> But I couldn't find "NR_FRAGMENT_PAGES" in linux-next.git .. is that vmstat counter? or others?
>
I mean rather than adding bunch vmstat counters for operations it might be
worth to add page counter which will show current amount of these pages.
But this seems too low-level for tracking, common counters for all network
buffers would be more useful but much harder to implement.
As I can see page owner is able to save stacktrace where allocation happened,
this makes debugging mostly trivial without any counters. If it adds too much
overhead - just track random 1% of pages, should be enough for finding leak.
> As you know, page_frag_alloc() directly calls __alloc_pages_nodemask() function,
> so that makes too difficult to see memory usage in real time even though we have "/meminfo or /slabinfo.." information.
> If there was a way already to figure out the memory leakage from page_frag_cache in mainline, I agree your opinion
> but I think we don't have it now.
>
> If those counters too much in my patch,
> I can say two values (pgfrag_alloc and pgfrag_free) are enough to guess what will happen
> and would remove pgfrag_alloc_calls and pgfrag_free_calls.
>
> Thanks,
> Kyeongdon Kim
>
> On 2017-09-01 오후 6:12, Konstantin Khlebnikov wrote:
>> IMHO that's too much counters.
>> Per-node NR_FRAGMENT_PAGES should be enough for guessing what's going on.
>> Perf probes provides enough features for furhter debugging.
>>
>> On 01.09.2017 02:37, Kyeongdon Kim wrote:
>> > There was a memory leak problem when we did stressful test
>> > on Android device.
>> > The root cause of this was from page_frag_cache alloc
>> > and it was very hard to find out.
>> >
>> > We add to count the page frag allocation and free with function call.
>> > The gap between pgfrag_alloc and pgfrag_free is good to to calculate
>> > for the amount of page.
>> > The gap between pgfrag_alloc_calls and pgfrag_free_calls is for
>> > sub-indicator.
>> > They can see trends of memory usage during the test.
>> > Without it, it's difficult to check page frag usage so I believe we
>> > should add it.
>> >
>> > Signed-off-by: Kyeongdon Kim <kyeongdon.kim@lge.com>
>> > ---
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Yang, Yi @ 2017-09-04 8:00 UTC (permalink / raw)
To: Jiri Benc
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170831124516.0c5db686@griffin>
On Thu, Aug 31, 2017 at 06:45:16PM +0800, Jiri Benc wrote:
> > + mask->context[i]);
> > + }
> > + memcpy(flow_key->nsh.context, nsh->md1.context,
> > + sizeof(nsh->md1.context));
>
> Do you follow the discussion that Hannes Sowa started on the ovs list
> regarding matching on the context fields? It would be better to hold on this
> until there's a conclusion reached.
>
I think we have had similiar discussion about this, the issue is we have
no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
So we have to do so, there is miniflow to handle such issue in
userspace, but kernel data path didn't provide such mechanism. I think
every newly-added key will result in the same space-wasting issue for
kernel data path, isn't it?
> > + err = nsh_key_from_nlattr(&attr.nla, &nsh);
> > + if (err)
> > + break;
> > + err = nsh_key_from_nlattr(&mask.nla, &nsh_mask);
> > + if (err)
> > + break;
>
> This is a lot of copying to just be able to use nla_for_each_nested. While
> I'm not a fan of how ovs uses the attributes to store both value and mask,
> it's not completely irrational. However, I don't think that the intent was
> to store a copy of the whole nested group. It's quite obvious that it does
> not fit the pattern from the gymnastics you need to do.
>
> Instead, store the mask in each of the nested attributes individually. There
> will be no need for the copying above and the code will get cleaner and
> faster. It's not that the nsh_key_from_nlattr function needs to be able to
> work separately for the key and mask. Nothing else than the line above uses
> this function. Just move the logic inside.
>
> Actually, it seems it can be moved directly to set_nsh.
OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
action, so no reference code for this, set action has two use cases, one
has mask but the other hasn't, so it will be easier an nested attribute is
handled as a whole, in user space, nsh_key_from_nlattr is used in
several places. If we change it per your proposal, there will be a lot
of rework, we have to provide two functions to handle this, one is for
the case with mask, the other is for the case without mask.
>
> > + err = set_nsh(skb, flow_key, &nsh, &nsh_mask);
> > + break;
> > + }
> > +
> > case OVS_KEY_ATTR_IPV4:
> > err = set_ipv4(skb, flow_key, nla_data(a),
> > get_mask(a, struct ovs_key_ipv4 *));
> > @@ -1210,6 +1336,21 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> > case OVS_ACTION_ATTR_POP_ETH:
> > err = pop_eth(skb, key);
> > break;
> > +
> > + case OVS_ACTION_ATTR_PUSH_NSH: {
> > + u8 buffer[NSH_HDR_MAX_LEN];
> > + struct nshhdr *nsh_hdr = (struct nshhdr *)buffer;
> > + const struct nshhdr *nsh_src = nsh_hdr;
> > +
> > + nsh_hdr_from_nlattr(nla_data(a), nsh_hdr,
> > + NSH_HDR_MAX_LEN);
>
> This is costly. This is called for every packet in the fast path. We should
> precompute and store the header instead.
How can we do so? I see batch process code in user space implementation,
but kernel data path didn't such infrastructure, how can we know next
push_nsh uses the same nsh header as previous one?
> > + case NSH_M_TYPE2:
> > + /* Don't support MD type 2 metedata parsing yet */
> > + if (length < NSH_BASE_HDR_LEN)
> > + return -EINVAL;
>
> Shouldn't we reject the packet, then? Pretending that something was parsed
> correctly while in fact it was not, does not look correct.
For MD type 2, we don't implement metadata parsing, but it can still
work. I'm not sure what you mean here, how do we reject a packet here?
>
> These checks should be done only once when the action is configured, not for
> each packet.
I don't know how to implement a batch processing in kernel data path, it
seems there isn't such code for reference.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/4] bpf: add helper bpf_perf_read_counter_time for perf event array map
From: Peter Zijlstra @ 2017-09-04 7:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: rostedt, ast, daniel, netdev, kernel-team
In-Reply-To: <20170902054824.371962-2-yhs@fb.com>
On Fri, Sep 01, 2017 at 10:48:21PM -0700, Yonghong Song wrote:
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index b14095b..5a50808 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -898,7 +898,8 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr,
> void *context);
> extern void perf_pmu_migrate_context(struct pmu *pmu,
> int src_cpu, int dst_cpu);
> -int perf_event_read_local(struct perf_event *event, u64 *value);
> +int perf_event_read_local(struct perf_event *event, u64 *value,
> + u64 *enabled, u64 *running);
> extern u64 perf_event_read_value(struct perf_event *event,
> u64 *enabled, u64 *running);
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8c01572..20c4039 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3670,7 +3670,8 @@ static inline u64 perf_event_count(struct perf_event *event)
> * will not be local and we cannot read them atomically
> * - must not have a pmu::count method
> */
> -int perf_event_read_local(struct perf_event *event, u64 *value)
> +int perf_event_read_local(struct perf_event *event, u64 *value,
> + u64 *enabled, u64 *running)
> {
> unsigned long flags;
> int ret = 0;
> @@ -3694,7 +3695,7 @@ int perf_event_read_local(struct perf_event *event, u64 *value)
> * It must not have a pmu::count method, those are not
> * NMI safe.
> */
> - if (event->pmu->count) {
> + if (value && event->pmu->count) {
> ret = -EOPNOTSUPP;
> goto out;
> }
No, value _must_ be !NULL. Otherwise you allow getting timestamps
independently from the count value and that is broken.
The {value, enabled, running} tuple is temporally related.
> @@ -3718,10 +3719,16 @@ int perf_event_read_local(struct perf_event *event, u64 *value)
> * or local to this CPU. Furthermore it means its ACTIVE (otherwise
> * oncpu == -1).
> */
> - if (event->oncpu == smp_processor_id())
> - event->pmu->read(event);
> -
> - *value = local64_read(&event->count);
> + if (value) {
> + if (event->oncpu == smp_processor_id())
> + event->pmu->read(event);
> + *value = local64_read(&event->count);
> + }
> + if (enabled && running) {
> + u64 ctx_time = event->shadow_ctx_time + perf_clock();
> + *enabled = ctx_time - event->tstamp_enabled;
> + *running = ctx_time - event->tstamp_running;
> + }
> out:
> local_irq_restore(flags);
Please make that something like:
u64 now = event->shadow_ctx_time + perf_clock();
if (enabled)
*enabled = now - event->tstamp_enabled;
if (event->oncpu == smp_processor_id()) {
event->pmu->read(event);
if (running)
*running = now - event->tstamp_running;
} else {
*running = event->total_time_running;
}
And I'll fix it up when I make:
https://lkml.kernel.org/r/20170831171837.njnc6r6elsvkl7lt@hirez.programming.kicks-ass.net
happen.
^ permalink raw reply
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