* 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: Jiri Benc @ 2017-09-04 13:16 UTC (permalink / raw)
To: Jan Scheurich
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D787F4E84-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
On Mon, 4 Sep 2017 12:57:15 +0000, Jan Scheurich wrote:
> 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
This needs to be:
OVS_KEY_ATTR_NSH
OVS_KEY_ATTR_NSH_BASE_HEADER
OVS_KEY_ATTR_NSH_MD1_CONTEXT
> 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
This needs to be:
OVS_ACTION_ATTR_SET_MASKED
OVS_KEY_ATTR_NSH
OVS_KEY_ATTR_NSH_BASE_HEADER
OVS_KEY_ATTR_NSH_MD1_CONTEXT
In your example, there's no way to distinguish OVS_KEY_ATTR_ENCAP and
OVS_KEY_ATTR_NSH_BASE_HEADER, they are both "1".
> 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.
I'm not sure. From the kernel point of view, we want the same
functionality offered by the openvswitch module and by a tc action.
In theory, it can be the tc tool that constructs the header from the
command line but there's no precedent. Dunno.
Jiri
^ permalink raw reply
* Re: EINVAL when using connect() for udp sockets
From: Daurnimator @ 2017-09-04 13:23 UTC (permalink / raw)
To: Eric Dumazet
Cc: Cong Wang, Linux Kernel Network Developers, William Ahern,
Santi T., Michael Kerrisk-manpages
In-Reply-To: <1493354932.31837.9.camel@edumazet-glaptop3.roam.corp.google.com>
On 28 April 2017 at 14:48, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2017-04-28 at 12:55 +1000, Daurnimator wrote:
>> On 1 April 2017 at 03:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > Please submit your patch formally and with a man page patch too.
>>
>> Did a patch get submitted? I had a look but couldn't see it.
>
> Not yet.
>
> I will probably wait for next cycle (linux-4.13), I have been busy on
> other work lately.
Will you have time to work on this soon?
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Hannes Frederic Sowa @ 2017-09-04 13:50 UTC (permalink / raw)
To: Tom Herbert
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <CALx6S356vRYfcsN6FaRmpn2oC0GN9-JwLsF7ybC+w4sEGGsa7w@mail.gmail.com>
Tom Herbert <tom@herbertland.com> writes:
> An encapsulator sets the UDP source port to be the flow entropy of the
> packet being encapsulated. So when the packet traverses the network
> devices can base their hash just on the canonical 5-tuple which is
> sufficient for ECMP and RSS. IPv6 flow label is even better since the
> middleboxes don't even need to look at the transport header, a packet
> is steered based on the 3-tuple of addresses and flow label. In the
> Linux stack, udp_flow_src_port is used by UDP encapsulations to set
> the source port. Flow label is similarly set by ip6_make_flowlabel.
> Both of these functions use the skb->hash which is computed by calling
> flow dissector at most once per packet (if the packet was received
> with an L4 HW hash or locally originated on a connection the hash does
> not need to be computed).
This would require the MPLS stack copying the flowlabel of IPv6
connections between MPLS routers over their whole lifetime in the MPLS
network. The same would hold for MPLS encapsulated inside UDP, the
source port needs to be kept constant. This is very impractical. The
hash for the flow label can often not be recomputed by interim routers,
because they might lack the knowledge of the upper layer protocol.
UDP source port entropy still has the problem that we don't respect the
source port as RSS entropy by default in network cards, because of
possible fragmentation and thus possible reordering of packets. GRE does
not have this problem and is way easier to identify by hardware.
Basically we need to tell network cards that they can use specific
destination UDP ports where we allow the source port to be used in RSS
hash calculation. I don't see how this is any easier than just using GRE
with a defined protocol field? I do like the combination of ipv6
flowlabel + GRE.
Btw. people are using the GRE Key as additional entropy without looking
into the GRE payload.
Bye,
Hannes
^ permalink raw reply
* RE: [PATCH net-next v7] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 14:07 UTC (permalink / raw)
To: Jiri Benc
Cc: Yang, Yi, netdev@vger.kernel.org, davem@davemloft.net,
dev@openvswitch.org, e@erig.me, blp@ovn.org
In-Reply-To: <20170904151601.2f198a53@griffin>
> > 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
>
> This needs to be:
>
> OVS_KEY_ATTR_NSH
> OVS_KEY_ATTR_NSH_BASE_HEADER
> OVS_KEY_ATTR_NSH_MD1_CONTEXT
>
> > 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
>
> This needs to be:
>
> OVS_ACTION_ATTR_SET_MASKED
> OVS_KEY_ATTR_NSH
> OVS_KEY_ATTR_NSH_BASE_HEADER
> OVS_KEY_ATTR_NSH_MD1_CONTEXT
>
Then perhaps I misunderstood your comment. I thought you didn't like that the
SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.
I was aiming to avoid this by lifting the two components of the NSH header
components to the top level:
OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER
OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT
> In your example, there's no way to distinguish OVS_KEY_ATTR_ENCAP and
> OVS_KEY_ATTR_NSH_BASE_HEADER, they are both "1".
See above. The two would be separate values in the same enum, i.e. distinct.
> > 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.
>
> I'm not sure. From the kernel point of view, we want the same
> functionality offered by the openvswitch module and by a tc action.
>
> In theory, it can be the tc tool that constructs the header from the
> command line but there's no precedent. Dunno.
Not sure I have the full picture here. Are you saying that the tc tool uses the same
Netlink API to the kernel as OVS? What would be the back-end for tc? Also the
openvswitch kernel module?
BR, Jan
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 14:13 UTC (permalink / raw)
To: Jan Scheurich
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D787F4EEB-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
On Mon, 4 Sep 2017 14:07:45 +0000, Jan Scheurich wrote:
> Then perhaps I misunderstood your comment. I thought you didn't like that the
> SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.
> I was aiming to avoid this by lifting the two components of the NSH header
> components to the top level:
> OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER
> OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT
No, this should be a nested attr.
I objected to the way value+mask combo is handled.
> See above. The two would be separate values in the same enum, i.e. distinct.
This is not how netlink attrs should be designed.
> Not sure I have the full picture here. Are you saying that the tc tool uses the same
> Netlink API to the kernel as OVS? What would be the back-end for tc? Also the
> openvswitch kernel module?
It does not use the same API but it makes sense for these two to share
common code. Plus hw offloading in ovs is done using tc.
Jiri
^ permalink raw reply
* [PATCH net-next] rxrpc: Make service connection lookup always check for retry
From: David Howells @ 2017-09-04 14:28 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
When an RxRPC service packet comes in, the target connection is looked up
by an rb-tree search under RCU and a read-locked seqlock; the seqlock retry
check is, however, currently skipped if we got a match, but probably
shouldn't be in case the connection we found gets replaced whilst we're
doing a search.
Make the lookup procedure always go through need_seqretry(), even if the
lookup was successful. This makes sure we always pick up on a write-lock
event.
On the other hand, since we don't take a ref on the object, but rely on RCU
to prevent its destruction after dropping the seqlock, I'm not sure this is
necessary.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/conn_service.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/rxrpc/conn_service.c b/net/rxrpc/conn_service.c
index e60fcd2a4a02..f6fcdb3130a1 100644
--- a/net/rxrpc/conn_service.c
+++ b/net/rxrpc/conn_service.c
@@ -50,12 +50,11 @@ struct rxrpc_connection *rxrpc_find_service_conn_rcu(struct rxrpc_peer *peer,
else if (conn->proto.index_key > k.index_key)
p = rcu_dereference_raw(p->rb_right);
else
- goto done;
+ break;
conn = NULL;
}
} while (need_seqretry(&peer->service_conn_lock, seq));
-done:
done_seqretry(&peer->service_conn_lock, seq);
_leave(" = %d", conn ? conn->debug_id : -1);
return conn;
^ permalink raw reply related
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 14:45 UTC (permalink / raw)
To: Jiri Benc
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20170904161303.69624386@griffin>
> On Mon, 4 Sep 2017 14:07:45 +0000, Jan Scheurich wrote:
> > Then perhaps I misunderstood your comment. I thought you didn't like that the
> > SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.
> > I was aiming to avoid this by lifting the two components of the NSH header
> > components to the top level:
> > OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER
> > OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT
>
> No, this should be a nested attr.
>
> I objected to the way value+mask combo is handled.
OK, sorry for the confusion.
So what is the correct layout for MASKED_SET action with nested fields?
1. All nested values, followed by all nested masks, or
2. For each nested field value followed by mask?
I guess alternative 1, but just to be sure.
Jan
^ permalink raw reply
* RE: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: David Laight @ 2017-09-04 14:49 UTC (permalink / raw)
To: 'Phil Sutter', Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20170901165256.21459-2-phil@nwl.cc>
From: Phil Sutter
> Sent: 01 September 2017 17:53
> By making use of strncpy(), both implementations are really simple so
> there is no need to add libbsd as additional dependency.
>
...
> +
> +size_t strlcpy(char *dst, const char *src, size_t size)
> +{
> + if (size) {
> + strncpy(dst, src, size - 1);
> + dst[size - 1] = '\0';
> + }
> + return strlen(src);
> +}
Except that isn't really strlcpy().
Better would be:
len = strlen(src) + 1;
if (len <= size)
memcpy(dst, src, len);
else if (size) {
dst[size - 1] = 0;
memcpy(dst, src, size - 1);
}
return len - 1;
WTF strlcpy() has that return value I don't know.
David
^ permalink raw reply
* [PATCH] net: ipv6: fix regression of no RTM_DELADDR sent after DAD failure
From: Mike Manning @ 2017-09-04 14:52 UTC (permalink / raw)
To: netdev; +Cc: Mahesh Bandewar
Commit f784ad3d79e5 ("ipv6: do not send RTM_DELADDR for tentative
addresses") incorrectly assumes that no RTM_NEWADDR are sent for
addresses in tentative state, as this does happen for the standard
IPv6 use-case of DAD failure, see the call to ipv6_ifa_notify() in
addconf_dad_stop(). So as a result of this change, no RTM_DELADDR is
sent after DAD failure for a link-local when strict DAD (accept_dad=2)
is configured, or on the next admin down in other cases. The absence
of this notification breaks backwards compatibility and causes problems
after DAD failure if this notification was being relied on. The
solution is to allow RTM_DELADDR to still be sent after DAD failure.
Fixes: f784ad3d79e5("ipv6: do not send RTM_DELADDR for tentative addresses")
Signed-off-by: Mike Manning <mmanning@brocade.com>
Cc: Mahesh Bandewar <maheshb@google.com>
---
net/ipv6/addrconf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 936e9ab..ba757c2 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4982,9 +4982,10 @@ static void inet6_ifa_notify(int event, struct inet6_ifaddr *ifa)
/* Don't send DELADDR notification for TENTATIVE address,
* since NEWADDR notification is sent only after removing
- * TENTATIVE flag.
+ * TENTATIVE flag, if DAD has not failed.
*/
- if (ifa->flags & IFA_F_TENTATIVE && event == RTM_DELADDR)
+ if (ifa->flags & IFA_F_TENTATIVE && !(ifa->flags & IFA_F_DADFAILED) &&
+ event == RTM_DELADDR)
return;
skb = nlmsg_new(inet6_ifaddr_msgsize(), GFP_ATOMIC);
--
2.1.4
^ permalink raw reply related
* Re: virtio_net: ethtool supported link modes
From: Radu Rendec @ 2017-09-04 14:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev
In-Reply-To: <20170901204222-mutt-send-email-mst@kernel.org>
On Fri, 2017-09-01 at 20:45 +0300, Michael S. Tsirkin wrote:
> On Fri, Sep 01, 2017 at 05:19:53PM +0100, Radu Rendec wrote:
> > On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> > > > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > > > and duplex can be set to any valid value. The driver will "remember"
> > > > them and report them back in virtnet_get_link_ksettings.
> > > >
> > > > However, the supported link modes (link_modes.supported in struct
> > > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > > > setting is supported.
> > > >
> > > > Does it make more sense to set (at least a few of) the supported link
> > > > modes, such as 10baseT_Half ... 10000baseT_Full?
> > > >
> > > > I would expect to see consistency between what is reported in
> > > > link_modes.supported and what can actually be set. Could you please
> > > > share your opinion on this?
> >
> > The use case behind my original question is very simple:
> > * Net device is queried via ethtool for supported modes.
> > * Supported modes are presented to user.
> > * User can configure any of the supported modes.
>
> Since this has no effect on virtio, isn't presenting
> "no supported modes" to user the right thing to do?
Yes, that makes sense.
> > This is done transparently to the net device type (driver), so it
> > actually makes sense for physical NICs.
> >
> > This alone of course is not a good enough motivation to modify the
> > driver. And it can be easily addressed in user-space at the application
> > level by testing for the driver.
>
> I think you might want to special-case no supported modes.
> Special-casing virtio is probably best avoided.
>
> > I was merely trying to avoid driver-specific workarounds (i.e. keep the
> > application driver agnostic)
>
> I think that's the right approach. So if driver does not present
> any supported modes this probably means it is not necessary
> to display or program any.
Yes, apparently it boils down to special-casing no supported modes.
This avoids both modifying virtio and special-casing virtio, and keeps
the application driver-agnostic at the same time.
Thanks for all the feedback. It was very helpful in figuring out the
right approach. I really appreciate it.
Radu
^ permalink raw reply
* Re: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: Phil Sutter @ 2017-09-04 15:00 UTC (permalink / raw)
To: David Laight; +Cc: Stephen Hemminger, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD006D878@AcuExch.aculab.com>
On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 01 September 2017 17:53
> > By making use of strncpy(), both implementations are really simple so
> > there is no need to add libbsd as additional dependency.
> >
> ...
> > +
> > +size_t strlcpy(char *dst, const char *src, size_t size)
> > +{
> > + if (size) {
> > + strncpy(dst, src, size - 1);
> > + dst[size - 1] = '\0';
> > + }
> > + return strlen(src);
> > +}
>
> Except that isn't really strlcpy().
> Better would be:
> len = strlen(src) + 1;
> if (len <= size)
> memcpy(dst, src, len);
> else if (size) {
> dst[size - 1] = 0;
> memcpy(dst, src, size - 1);
> }
> return len - 1;
Please elaborate: Why isn't my version "really" strlcpy()? Why is your
proposed version better?
Thanks, Phil
^ permalink raw reply
* Re: [PATCH v06 35/36] uapi linux/tls.h: don't include <net/tcp.h> in user space
From: Dmitry V. Levin @ 2017-09-04 16:15 UTC (permalink / raw)
To: linux-kernel, linux-api, Mikko Rapeli, Dave Watson, Ilya Lesokhin,
Aviad Yehezkel, netdev
In-Reply-To: <20170808232554.GK10552@altlinux.org>
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]
On Wed, Aug 09, 2017 at 02:25:54AM +0300, Dmitry V. Levin wrote:
> On Sun, Aug 06, 2017 at 06:44:26PM +0200, Mikko Rapeli wrote:
> > It is not needed and not part of uapi headers, but causes
> > user space compilation error:
> >
> > fatal error: net/tcp.h: No such file or directory
> > #include <net/tcp.h>
> > ^
> >
> > Signed-off-by: Mikko Rapeli <mikko.rapeli@iki.fi>
> > Cc: Dave Watson <davejwatson@fb.com>
> > Cc: Ilya Lesokhin <ilyal@mellanox.com>
> > Cc: Aviad Yehezkel <aviadye@mellanox.com>
> > ---
> > include/uapi/linux/tls.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
> > index cc1d21db35d8..d87c698623f2 100644
> > --- a/include/uapi/linux/tls.h
> > +++ b/include/uapi/linux/tls.h
> > @@ -37,7 +37,9 @@
> > #include <asm/byteorder.h>
> > #include <linux/socket.h>
> > #include <linux/tcp.h>
> > +#ifdef __KERNEL__
> > #include <net/tcp.h>
> > +#endif
>
> Let's move it to include/net/tls.h instead.
So everybody ignored this and *new* uapi header was released
in a totally unusable form along with v4.13.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Tom Herbert @ 2017-09-04 16:15 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <87pob6bmjv.fsf@stressinduktion.org>
On Mon, Sep 4, 2017 at 6:50 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Tom Herbert <tom@herbertland.com> writes:
>
>> An encapsulator sets the UDP source port to be the flow entropy of the
>> packet being encapsulated. So when the packet traverses the network
>> devices can base their hash just on the canonical 5-tuple which is
>> sufficient for ECMP and RSS. IPv6 flow label is even better since the
>> middleboxes don't even need to look at the transport header, a packet
>> is steered based on the 3-tuple of addresses and flow label. In the
>> Linux stack, udp_flow_src_port is used by UDP encapsulations to set
>> the source port. Flow label is similarly set by ip6_make_flowlabel.
>> Both of these functions use the skb->hash which is computed by calling
>> flow dissector at most once per packet (if the packet was received
>> with an L4 HW hash or locally originated on a connection the hash does
>> not need to be computed).
>
> This would require the MPLS stack copying the flowlabel of IPv6
> connections between MPLS routers over their whole lifetime in the MPLS
> network. The same would hold for MPLS encapsulated inside UDP, the
> source port needs to be kept constant. This is very impractical. The
> hash for the flow label can often not be recomputed by interim routers,
> because they might lack the knowledge of the upper layer protocol.
>
Hannes,
When the flow label is set the packet will traverse the network and be
ECMP routed regardless of whether the payload is MPLS at anything
else-- the important characteristic is that network devices don't need
to know how to parse MPLS (or GRE, or IPIP, or L2TP, ESP, or ...) to
provide good ECMP. At a source the flow label or UDP source port needs
to be generated. That can be based on DPI, derived from the MPLS
entropy label, use SPI in ESP, etc. I don't see anything special about
MPLS in this regard.
> UDP source port entropy still has the problem that we don't respect the
> source port as RSS entropy by default in network cards, because of
> possible fragmentation and thus possible reordering of packets. GRE does
> not have this problem and is way easier to identify by hardware.
>
> Basically we need to tell network cards that they can use specific
> destination UDP ports where we allow the source port to be used in RSS
> hash calculation. I don't see how this is any easier than just using GRE
> with a defined protocol field? I do like the combination of ipv6
> flowlabel + GRE.
>
No, we don't any more want port specific configuration in NICs! The
NIC should just fallback to 3-tuple hash when it see MF or offset set
in IPv4 header. But even if it doesn't implement this, receiving OOO
fragments is hardly the end of the world-- IP packets are always
allowed to be received OOO. If something breaks because in order
delivery is assumed then that is the bug that needs to be fixed. So at
best handling fragmentation in this manner is proposed om
optimization whose benefits will pale to getting good ECMP and RSS
when encapsulation is in use.
> Btw. people are using the GRE Key as additional entropy without looking
> into the GRE payload.
>
Sure some are, but the GRE key is not defined to be flow entropy so
it's not ubiquitous it used for that so it gives sufficient entropy or
is even constant per flow. GRE/UDP (RFC8086) was primarily written to
allow a more consistent method (as was RFC7510 for doing MPLS/UDP).
Tom
^ permalink raw reply
* Re: [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
From: Pavel Machek @ 2017-09-04 16:25 UTC (permalink / raw)
To: Matthew Tan
Cc: jeffrey.t.kirsher, michael.kardonik, carolyn.wyborny,
donald.c.skidmore, bruce.w.allan, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, linux-kernel
In-Reply-To: <1503503985-3869-1-git-send-email-matthew.tan_1@nxp.com>
[-- Attachment #1: Type: text/plain, Size: 814 bytes --]
Hi!
> @@ -183,7 +183,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
> * reading duplicate data in the next MDIC transaction.
> */
> if (hw->mac.type == e1000_pch2lan)
> - udelay(100);
> + usleep_range(90, 100);
>
> return 0;
> }
Can you explain why shortening the delay is acceptable here?
> @@ -246,7 +246,7 @@ s32 e1000e_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
> * reading duplicate data in the next MDIC transaction.
> */
> if (hw->mac.type == e1000_pch2lan)
> - udelay(100);
> + usleep_range(90, 110);
>
> return 0;
> }
And here?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH 1/1] net: mdio-mux: add mdio_mux parameter to mdio_mux_init()
From: Corentin Labbe @ 2017-09-04 16:30 UTC (permalink / raw)
To: andrew, f.fainelli, rjui, sbranden, jonmason
Cc: bcm-kernel-feedback-list, netdev, linux-arm-kernel, linux-kernel,
Corentin Labbe
In-Reply-To: <20170904163014.955-1-clabbe.montjoie@gmail.com>
mdio_mux_init() use the parameter dev for two distinct thing:
1) Have a device for all devm_ functions
2) Get device_node from it
Since it is two distinct purpose, this patch add a parameter mdio_mux
that is linked to task 2.
This will also permit to register an of_node mdio-mux that lacks a direct
owning device.
For example a mdio-mux which is a subnode of a real device.
Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
drivers/net/phy/mdio-mux-bcm-iproc.c | 2 +-
drivers/net/phy/mdio-mux-gpio.c | 2 +-
drivers/net/phy/mdio-mux-mmioreg.c | 3 ++-
drivers/net/phy/mdio-mux.c | 7 ++++---
include/linux/mdio-mux.h | 9 +++++++++
5 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 0a5f62e0efcc..0831b7142df7 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -199,7 +199,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, md);
- rc = mdio_mux_init(md->dev, mdio_mux_iproc_switch_fn,
+ rc = mdio_mux_init(md->dev, md->dev->of_node, mdio_mux_iproc_switch_fn,
&md->mux_handle, md, md->mii_bus);
if (rc) {
dev_info(md->dev, "mdiomux initialization failed\n");
diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
index 919949960a10..082ffef0dec4 100644
--- a/drivers/net/phy/mdio-mux-gpio.c
+++ b/drivers/net/phy/mdio-mux-gpio.c
@@ -54,7 +54,7 @@ static int mdio_mux_gpio_probe(struct platform_device *pdev)
if (IS_ERR(s->gpios))
return PTR_ERR(s->gpios);
- r = mdio_mux_init(&pdev->dev,
+ r = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
mdio_mux_gpio_switch_fn, &s->mux_handle, s, NULL);
if (r != 0) {
diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
index c3825c7da038..2573ab012f16 100644
--- a/drivers/net/phy/mdio-mux-mmioreg.c
+++ b/drivers/net/phy/mdio-mux-mmioreg.c
@@ -159,7 +159,8 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
}
}
- ret = mdio_mux_init(&pdev->dev, mdio_mux_mmioreg_switch_fn,
+ ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
+ mdio_mux_mmioreg_switch_fn,
&s->mux_handle, s, NULL);
if (ret) {
dev_err(&pdev->dev, "failed to register mdio-mux bus %pOF\n",
diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
index 6f75e9f27fed..0a86f1e4c02f 100644
--- a/drivers/net/phy/mdio-mux.c
+++ b/drivers/net/phy/mdio-mux.c
@@ -86,6 +86,7 @@ static int mdio_mux_write(struct mii_bus *bus, int phy_id,
static int parent_count;
int mdio_mux_init(struct device *dev,
+ struct device_node *mux_node,
int (*switch_fn)(int cur, int desired, void *data),
void **mux_handle,
void *data,
@@ -98,11 +99,11 @@ int mdio_mux_init(struct device *dev,
struct mdio_mux_parent_bus *pb;
struct mdio_mux_child_bus *cb;
- if (!dev->of_node)
+ if (!mux_node)
return -ENODEV;
if (!mux_bus) {
- parent_bus_node = of_parse_phandle(dev->of_node,
+ parent_bus_node = of_parse_phandle(mux_node,
"mdio-parent-bus", 0);
if (!parent_bus_node)
@@ -132,7 +133,7 @@ int mdio_mux_init(struct device *dev,
pb->mii_bus = parent_bus;
ret_val = -ENODEV;
- for_each_available_child_of_node(dev->of_node, child_bus_node) {
+ for_each_available_child_of_node(mux_node, child_bus_node) {
int v;
r = of_property_read_u32(child_bus_node, "reg", &v);
diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
index 61f5b21b31c7..a5d58f221939 100644
--- a/include/linux/mdio-mux.h
+++ b/include/linux/mdio-mux.h
@@ -12,7 +12,16 @@
#include <linux/device.h>
#include <linux/phy.h>
+/* mdio_mux_init() - Initialize a MDIO mux
+ * @dev The device owning the MDIO mux
+ * @mux_node The device node of the MDIO mux
+ * @switch_fn The function called for switching target MDIO child
+ * mux_handle A pointer to a (void *) used internaly by mdio-mux
+ * @data Private data used by switch_fn()
+ * @mux_bus An optional parent bus (Other case are to use parent_bus property)
+ */
int mdio_mux_init(struct device *dev,
+ struct device_node *mux_node,
int (*switch_fn) (int cur, int desired, void *data),
void **mux_handle,
void *data,
--
2.13.5
^ permalink raw reply related
* [PATCH 0/1] net: mdio-mux: add mdio_mux parameter to mdio_mux_init()
From: Corentin Labbe @ 2017-09-04 16:30 UTC (permalink / raw)
To: andrew, f.fainelli, rjui, sbranden, jonmason
Cc: bcm-kernel-feedback-list, netdev, linux-arm-kernel, linux-kernel,
Corentin Labbe
Hello
For dwmac-sun8i, we need to set a MDIO mux which is not itself a device
(it is part of a device but have its own DT node)
This patch permit to use a MDIO mux for such case.
See agreement at https://lkml.org/lkml/2017/8/29/407
Since all mdio_mux_init() users are within drivers/net/phy/ and the changes are
trivial, I have done all in one patch, instead of having a complex series of
patch converting one by one callers.
This patch is a dependency for restoring dwmac-sun8i so it is why I target "net:"
But I send it alone, for be sure that this conversion in one patch is acceptable.
Regards
Corentin Labbe (1):
net: mdio-mux: add mdio_mux parameter to mdio_mux_init()
drivers/net/phy/mdio-mux-bcm-iproc.c | 2 +-
drivers/net/phy/mdio-mux-gpio.c | 2 +-
drivers/net/phy/mdio-mux-mmioreg.c | 3 ++-
drivers/net/phy/mdio-mux.c | 7 ++++---
include/linux/mdio-mux.h | 9 +++++++++
5 files changed, 17 insertions(+), 6 deletions(-)
--
2.13.5
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
From: Paul Menzel @ 2017-09-04 16:36 UTC (permalink / raw)
To: Matthew Tan
Cc: jeffrey.t.kirsher, michael.kardonik, mitch.a.williams,
linux-kernel, john.ronciak, intel-wired-lan, netdev
In-Reply-To: <1503503985-3869-1-git-send-email-matthew.tan_1@nxp.com>
Dear Matthew,
On 08/23/17 17:59, Matthew Tan wrote:
> Calls to udelay are not preemtable by userspace so userspace
> applications experience a large (~200us) latency when running on core
> 0. Instead usleep_range can be used to be more friendly to userspace
> since it is preemtable. This is due to udelay using busy-wait loops
> while usleep_rang uses hrtimers instead. It is recommended to use
> udelay when the delay is <10us since at that precision overhead of
> usleep_range hrtimer setup causes issues. However, the replaced calls
> are for 50us and 100us so this should not be not an issue.
Is there a reason for indenting the paragraph. (I guess, you did `git
show` or `git log -p` and copied the message?) Anyway, please remove
whitespace in the front, if there is no reason.
Also, it looks like you ran benchmarks, so what is the delay for
userspace applications with your changes?
> Signed-off-by: Matthew Tan <matthew.tan_1@nxp.com>
> ---
> drivers/net/ethernet/intel/e1000e/phy.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
[…]
Kind regards,
Paul
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Hannes Frederic Sowa @ 2017-09-04 16:52 UTC (permalink / raw)
To: Tom Herbert
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <CALx6S37wcp1om0Dwr1=-6jF=HkAuZj313pLJ7sVhgsyJ4yZDjg@mail.gmail.com>
Hello Tom,
Tom Herbert <tom@herbertland.com> writes:
> On Mon, Sep 4, 2017 at 6:50 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Tom Herbert <tom@herbertland.com> writes:
>>
>>> An encapsulator sets the UDP source port to be the flow entropy of the
>>> packet being encapsulated. So when the packet traverses the network
>>> devices can base their hash just on the canonical 5-tuple which is
>>> sufficient for ECMP and RSS. IPv6 flow label is even better since the
>>> middleboxes don't even need to look at the transport header, a packet
>>> is steered based on the 3-tuple of addresses and flow label. In the
>>> Linux stack, udp_flow_src_port is used by UDP encapsulations to set
>>> the source port. Flow label is similarly set by ip6_make_flowlabel.
>>> Both of these functions use the skb->hash which is computed by calling
>>> flow dissector at most once per packet (if the packet was received
>>> with an L4 HW hash or locally originated on a connection the hash does
>>> not need to be computed).
>>
>> This would require the MPLS stack copying the flowlabel of IPv6
>> connections between MPLS routers over their whole lifetime in the MPLS
>> network. The same would hold for MPLS encapsulated inside UDP, the
>> source port needs to be kept constant. This is very impractical. The
>> hash for the flow label can often not be recomputed by interim routers,
>> because they might lack the knowledge of the upper layer protocol.
>>
> Hannes,
>
> When the flow label is set the packet will traverse the network and be
> ECMP routed regardless of whether the payload is MPLS at anything
> else-- the important characteristic is that network devices don't need
> to know how to parse MPLS (or GRE, or IPIP, or L2TP, ESP, or ...) to
> provide good ECMP. At a source the flow label or UDP source port needs
> to be generated. That can be based on DPI, derived from the MPLS
> entropy label, use SPI in ESP, etc. I don't see anything special about
> MPLS in this regard.
The MPLS circuit is only end to end in terms of IP processing if MPLS is
used for multitenant separation.
Normally the IP connection is done between two label switch routers,
thus is not end to end. One LSR will decapsulate the packet and throw
the IP header away, do the label processing and will reencapsulate it
with the new next hop information. To keep the assigned entropy alive it
would have to save the UDP source port or flowlabel and patch the
outgoing IP header again. This is certainly possible it just seems more
unnatural.
Normally every next hop does MPLS processing and thus the packet
traverses up the stack. Special purpose (entropy) MPLS labels allow the
stack to achieve RSS just based on the label stack and will be
end-to-end in a MPLS cloud.
>> UDP source port entropy still has the problem that we don't respect the
>> source port as RSS entropy by default in network cards, because of
>> possible fragmentation and thus possible reordering of packets. GRE does
>> not have this problem and is way easier to identify by hardware.
>>
>> Basically we need to tell network cards that they can use specific
>> destination UDP ports where we allow the source port to be used in RSS
>> hash calculation. I don't see how this is any easier than just using GRE
>> with a defined protocol field? I do like the combination of ipv6
>> flowlabel + GRE.
>>
> No, we don't any more want port specific configuration in NICs! The
> NIC should just fallback to 3-tuple hash when it see MF or offset set
> in IPv4 header. But even if it doesn't implement this, receiving OOO
> fragments is hardly the end of the world-- IP packets are always
> allowed to be received OOO. If something breaks because in order
> delivery is assumed then that is the bug that needs to be fixed. So at
> best handling fragmentation in this manner is proposed om
> optimization whose benefits will pale to getting good ECMP and RSS
> when encapsulation is in use.
The problem is that you end up having two streams, one fragmented and
one non-fragmented, but actually they belong to the same stream. It is
known to break stuff, see:
<https://patchwork.ozlabs.org/patch/59235/>
I would agree with you, but we can't break existing setups,
unfortunately.
>> Btw. people are using the GRE Key as additional entropy without looking
>> into the GRE payload.
>>
> Sure some are, but the GRE key is not defined to be flow entropy so
> it's not ubiquitous it used for that so it gives sufficient entropy or
> is even constant per flow. GRE/UDP (RFC8086) was primarily written to
> allow a more consistent method (as was RFC7510 for doing MPLS/UDP).
I agree with that, just wanted to mention it.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/1] net: mdio-mux: add mdio_mux parameter to mdio_mux_init()
From: Florian Fainelli @ 2017-09-04 17:00 UTC (permalink / raw)
To: Corentin Labbe, andrew, rjui, sbranden, jonmason
Cc: bcm-kernel-feedback-list, netdev, linux-arm-kernel, linux-kernel
In-Reply-To: <20170904163014.955-2-clabbe.montjoie@gmail.com>
On 09/04/2017 09:30 AM, Corentin Labbe wrote:
> mdio_mux_init() use the parameter dev for two distinct thing:
> 1) Have a device for all devm_ functions
> 2) Get device_node from it
>
> Since it is two distinct purpose, this patch add a parameter mdio_mux
> that is linked to task 2.
>
> This will also permit to register an of_node mdio-mux that lacks a direct
> owning device.
> For example a mdio-mux which is a subnode of a real device.
This looks fine, I was going to suggest introducing a wrapper around
mdio_mux_init() which does something along the lines below, but
considering the number of users of mdio_mux_init() within the kernel
tree, it's reasonably easy to audit those, therefore:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
index 61f5b21b31c7..0316186c91b5 100644
--- a/include/linux/mdio-mux.h
+++ b/include/linux/mdio-mux.h
@@ -12,11 +12,19 @@
#include <linux/device.h>
#include <linux/phy.h>
-int mdio_mux_init(struct device *dev,
- int (*switch_fn) (int cur, int desired, void *data),
- void **mux_handle,
- void *data,
- struct mii_bus *mux_bus);
+int mdio_mux_init_dn(struct device *dev, struct device_node *mux_node,
+ int (*switch_fn)(int cur, int desired, void *data),
+ void **mux_handle, void *data,
+ struct mii_bus *mux_bus);
+
+static inline int mdio_mux_init(struct device *dev,
+ int (*switch_fn)(int cur, int desired,
void *data),
+ void **mux_handle, void *data,
+ struct mii_bus *mux_bus)
+{
+ return mdio_mux_init_dn(dev, dev->of_node, switch_fn,
+ mux_handle, data, mux_bus);
+}
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> drivers/net/phy/mdio-mux-bcm-iproc.c | 2 +-
> drivers/net/phy/mdio-mux-gpio.c | 2 +-
> drivers/net/phy/mdio-mux-mmioreg.c | 3 ++-
> drivers/net/phy/mdio-mux.c | 7 ++++---
> include/linux/mdio-mux.h | 9 +++++++++
> 5 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
> index 0a5f62e0efcc..0831b7142df7 100644
> --- a/drivers/net/phy/mdio-mux-bcm-iproc.c
> +++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
> @@ -199,7 +199,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, md);
>
> - rc = mdio_mux_init(md->dev, mdio_mux_iproc_switch_fn,
> + rc = mdio_mux_init(md->dev, md->dev->of_node, mdio_mux_iproc_switch_fn,
> &md->mux_handle, md, md->mii_bus);
> if (rc) {
> dev_info(md->dev, "mdiomux initialization failed\n");
> diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
> index 919949960a10..082ffef0dec4 100644
> --- a/drivers/net/phy/mdio-mux-gpio.c
> +++ b/drivers/net/phy/mdio-mux-gpio.c
> @@ -54,7 +54,7 @@ static int mdio_mux_gpio_probe(struct platform_device *pdev)
> if (IS_ERR(s->gpios))
> return PTR_ERR(s->gpios);
>
> - r = mdio_mux_init(&pdev->dev,
> + r = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> mdio_mux_gpio_switch_fn, &s->mux_handle, s, NULL);
>
> if (r != 0) {
> diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
> index c3825c7da038..2573ab012f16 100644
> --- a/drivers/net/phy/mdio-mux-mmioreg.c
> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
> @@ -159,7 +159,8 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
> }
> }
>
> - ret = mdio_mux_init(&pdev->dev, mdio_mux_mmioreg_switch_fn,
> + ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> + mdio_mux_mmioreg_switch_fn,
> &s->mux_handle, s, NULL);
> if (ret) {
> dev_err(&pdev->dev, "failed to register mdio-mux bus %pOF\n",
> diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
> index 6f75e9f27fed..0a86f1e4c02f 100644
> --- a/drivers/net/phy/mdio-mux.c
> +++ b/drivers/net/phy/mdio-mux.c
> @@ -86,6 +86,7 @@ static int mdio_mux_write(struct mii_bus *bus, int phy_id,
> static int parent_count;
>
> int mdio_mux_init(struct device *dev,
> + struct device_node *mux_node,
> int (*switch_fn)(int cur, int desired, void *data),
> void **mux_handle,
> void *data,
> @@ -98,11 +99,11 @@ int mdio_mux_init(struct device *dev,
> struct mdio_mux_parent_bus *pb;
> struct mdio_mux_child_bus *cb;
>
> - if (!dev->of_node)
> + if (!mux_node)
> return -ENODEV;
>
> if (!mux_bus) {
> - parent_bus_node = of_parse_phandle(dev->of_node,
> + parent_bus_node = of_parse_phandle(mux_node,
> "mdio-parent-bus", 0);
>
> if (!parent_bus_node)
> @@ -132,7 +133,7 @@ int mdio_mux_init(struct device *dev,
> pb->mii_bus = parent_bus;
>
> ret_val = -ENODEV;
> - for_each_available_child_of_node(dev->of_node, child_bus_node) {
> + for_each_available_child_of_node(mux_node, child_bus_node) {
> int v;
>
> r = of_property_read_u32(child_bus_node, "reg", &v);
> diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
> index 61f5b21b31c7..a5d58f221939 100644
> --- a/include/linux/mdio-mux.h
> +++ b/include/linux/mdio-mux.h
> @@ -12,7 +12,16 @@
> #include <linux/device.h>
> #include <linux/phy.h>
>
> +/* mdio_mux_init() - Initialize a MDIO mux
> + * @dev The device owning the MDIO mux
> + * @mux_node The device node of the MDIO mux
> + * @switch_fn The function called for switching target MDIO child
> + * mux_handle A pointer to a (void *) used internaly by mdio-mux
> + * @data Private data used by switch_fn()
> + * @mux_bus An optional parent bus (Other case are to use parent_bus property)
> + */
> int mdio_mux_init(struct device *dev,
> + struct device_node *mux_node,
> int (*switch_fn) (int cur, int desired, void *data),
> void **mux_handle,
> void *data,
>
--
Florian
^ permalink raw reply related
* Re: [PATCH 1/1] net: mdio-mux: add mdio_mux parameter to mdio_mux_init()
From: Florian Fainelli @ 2017-09-04 17:00 UTC (permalink / raw)
To: Corentin Labbe, andrew, rjui, sbranden, jonmason
Cc: bcm-kernel-feedback-list, netdev, linux-arm-kernel, linux-kernel
In-Reply-To: <20170904163014.955-2-clabbe.montjoie@gmail.com>
On 09/04/2017 09:30 AM, Corentin Labbe wrote:
> mdio_mux_init() use the parameter dev for two distinct thing:
> 1) Have a device for all devm_ functions
> 2) Get device_node from it
>
> Since it is two distinct purpose, this patch add a parameter mdio_mux
> that is linked to task 2.
>
> This will also permit to register an of_node mdio-mux that lacks a direct
> owning device.
> For example a mdio-mux which is a subnode of a real device.
This looks fine, I was going to suggest introducing a wrapper around
mdio_mux_init() which does something along the lines below, but
considering the number of users of mdio_mux_init() within the kernel
tree, it's reasonably easy to audit those, therefore:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
index 61f5b21b31c7..0316186c91b5 100644
--- a/include/linux/mdio-mux.h
+++ b/include/linux/mdio-mux.h
@@ -12,11 +12,19 @@
#include <linux/device.h>
#include <linux/phy.h>
-int mdio_mux_init(struct device *dev,
- int (*switch_fn) (int cur, int desired, void *data),
- void **mux_handle,
- void *data,
- struct mii_bus *mux_bus);
+int mdio_mux_init_dn(struct device *dev, struct device_node *mux_node,
+ int (*switch_fn)(int cur, int desired, void *data),
+ void **mux_handle, void *data,
+ struct mii_bus *mux_bus);
+
+static inline int mdio_mux_init(struct device *dev,
+ int (*switch_fn)(int cur, int desired,
void *data),
+ void **mux_handle, void *data,
+ struct mii_bus *mux_bus)
+{
+ return mdio_mux_init_dn(dev, dev->of_node, switch_fn,
+ mux_handle, data, mux_bus);
+}
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> drivers/net/phy/mdio-mux-bcm-iproc.c | 2 +-
> drivers/net/phy/mdio-mux-gpio.c | 2 +-
> drivers/net/phy/mdio-mux-mmioreg.c | 3 ++-
> drivers/net/phy/mdio-mux.c | 7 ++++---
> include/linux/mdio-mux.h | 9 +++++++++
> 5 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
> index 0a5f62e0efcc..0831b7142df7 100644
> --- a/drivers/net/phy/mdio-mux-bcm-iproc.c
> +++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
> @@ -199,7 +199,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, md);
>
> - rc = mdio_mux_init(md->dev, mdio_mux_iproc_switch_fn,
> + rc = mdio_mux_init(md->dev, md->dev->of_node, mdio_mux_iproc_switch_fn,
> &md->mux_handle, md, md->mii_bus);
> if (rc) {
> dev_info(md->dev, "mdiomux initialization failed\n");
> diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
> index 919949960a10..082ffef0dec4 100644
> --- a/drivers/net/phy/mdio-mux-gpio.c
> +++ b/drivers/net/phy/mdio-mux-gpio.c
> @@ -54,7 +54,7 @@ static int mdio_mux_gpio_probe(struct platform_device *pdev)
> if (IS_ERR(s->gpios))
> return PTR_ERR(s->gpios);
>
> - r = mdio_mux_init(&pdev->dev,
> + r = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> mdio_mux_gpio_switch_fn, &s->mux_handle, s, NULL);
>
> if (r != 0) {
> diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
> index c3825c7da038..2573ab012f16 100644
> --- a/drivers/net/phy/mdio-mux-mmioreg.c
> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
> @@ -159,7 +159,8 @@ static int mdio_mux_mmioreg_probe(struct platform_device *pdev)
> }
> }
>
> - ret = mdio_mux_init(&pdev->dev, mdio_mux_mmioreg_switch_fn,
> + ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> + mdio_mux_mmioreg_switch_fn,
> &s->mux_handle, s, NULL);
> if (ret) {
> dev_err(&pdev->dev, "failed to register mdio-mux bus %pOF\n",
> diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
> index 6f75e9f27fed..0a86f1e4c02f 100644
> --- a/drivers/net/phy/mdio-mux.c
> +++ b/drivers/net/phy/mdio-mux.c
> @@ -86,6 +86,7 @@ static int mdio_mux_write(struct mii_bus *bus, int phy_id,
> static int parent_count;
>
> int mdio_mux_init(struct device *dev,
> + struct device_node *mux_node,
> int (*switch_fn)(int cur, int desired, void *data),
> void **mux_handle,
> void *data,
> @@ -98,11 +99,11 @@ int mdio_mux_init(struct device *dev,
> struct mdio_mux_parent_bus *pb;
> struct mdio_mux_child_bus *cb;
>
> - if (!dev->of_node)
> + if (!mux_node)
> return -ENODEV;
>
> if (!mux_bus) {
> - parent_bus_node = of_parse_phandle(dev->of_node,
> + parent_bus_node = of_parse_phandle(mux_node,
> "mdio-parent-bus", 0);
>
> if (!parent_bus_node)
> @@ -132,7 +133,7 @@ int mdio_mux_init(struct device *dev,
> pb->mii_bus = parent_bus;
>
> ret_val = -ENODEV;
> - for_each_available_child_of_node(dev->of_node, child_bus_node) {
> + for_each_available_child_of_node(mux_node, child_bus_node) {
> int v;
>
> r = of_property_read_u32(child_bus_node, "reg", &v);
> diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
> index 61f5b21b31c7..a5d58f221939 100644
> --- a/include/linux/mdio-mux.h
> +++ b/include/linux/mdio-mux.h
> @@ -12,7 +12,16 @@
> #include <linux/device.h>
> #include <linux/phy.h>
>
> +/* mdio_mux_init() - Initialize a MDIO mux
> + * @dev The device owning the MDIO mux
> + * @mux_node The device node of the MDIO mux
> + * @switch_fn The function called for switching target MDIO child
> + * mux_handle A pointer to a (void *) used internaly by mdio-mux
> + * @data Private data used by switch_fn()
> + * @mux_bus An optional parent bus (Other case are to use parent_bus property)
> + */
> int mdio_mux_init(struct device *dev,
> + struct device_node *mux_node,
> int (*switch_fn) (int cur, int desired, void *data),
> void **mux_handle,
> void *data,
>
--
Florian
^ permalink raw reply related
* Mutual Coperation Thank you
From: Mr bassole Obama @ 2017-09-04 17:00 UTC (permalink / raw)
--
Dear Friend,
I know that this message will come to you as a surprise. I am the
Auditing and Accounting section manager with African Development Bank,
Ouagadougou Burkina faso. I Hope that you will not expose or betray
this trust and confident that I am about to repose on you for the
mutual benefit of our both families.
I need your urgent assistance in transferring the sum of($39.5)million
to your account within 10 or 14 banking days. This money has been
dormant for years in our Bank without claim.I want the bank to release
the money to you as the nearest person to our deceased customer late
George small. who died along with his supposed next of kin in an air
crash since 31st October 1999.
I don't want the money to go into government treasury as an abandoned
fund. So this is the reason why I am contacting you so that the bank
can release the money to you as the next of kin to the deceased
customer. Please I would like you to keep this proposal as atop secret
and delete it if you are not interested.
Upon receipt of your reply, I will give you full details on how the
business will be executed and also note that you will have 40% of the
above mentioned sum if you agree to handle this business with me.
I am expecting your urgent response as soon as you receive my message.
Best Regard,
Auditor Mr Bassole Obama
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Tom Herbert @ 2017-09-04 17:11 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <8760cytnif.fsf@stressinduktion.org>
> The problem is that you end up having two streams, one fragmented and
> one non-fragmented, but actually they belong to the same stream. It is
> known to break stuff, see:
>
> <https://patchwork.ozlabs.org/patch/59235/>
>
> I would agree with you, but we can't break existing setups,
> unfortunately.
>
I'm not sure what "existing setups" means here. If this is referring
to the Internet then out of order packets and fragments abound-- any
assumption of in order delivery for IP packets is useless there.
Btw, TCP has exactly the same problem in this regard that UDP has with
regard to fragmentation. The reason that TCP isn't considered an issue
is the assumption that TCP will do PMTU discovery and set DF (I would
bet that devices don't actually check for DF and vendors don't test
when it's not set!).
Tom
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Hannes Frederic Sowa @ 2017-09-04 17:57 UTC (permalink / raw)
To: Tom Herbert
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <CALx6S34cOL4Gy9DMWnLV0iCVUYo+siMEeNNn9v5tBHr2P6yjYw@mail.gmail.com>
Hi Tom,
Tom Herbert <tom@herbertland.com> writes:
>> The problem is that you end up having two streams, one fragmented and
>> one non-fragmented, but actually they belong to the same stream. It is
>> known to break stuff, see:
>>
>> <https://patchwork.ozlabs.org/patch/59235/>
>>
>> I would agree with you, but we can't break existing setups,
>> unfortunately.
>>
> I'm not sure what "existing setups" means here. If this is referring
> to the Internet then out of order packets and fragments abound-- any
> assumption of in order delivery for IP packets is useless there.
Network cards don't know where traffic originates from, even on the same
card. Clouds nowadays try to convince everyone to virtualize existing
workloads. So I *assume* that at least for traffic that seems to be in
one L2 domain out of order should not occur or things will break.
Ethernet for a long time appeared to do very much in order delivery and
guarantees that. Thus for people it appeared that UDP packets are also
strictly in order. Encapsulating Ethernet inside UDP thus must preserve
those properties, especially if used for virtualization. Unfortunately
fragmentation happens and the stack has to deal with it somehow.
I do know about some software that uses UDP in multicast that is prone
to misbehave in case of OoO frames. It uses a small reassemble queue but
if reordering gets too high, it will simply drop data. This might be
acceptable up to a specific degree.
I guess one application you could harm is plain old simple syslog, which
often generated fragmented packets. Luckily one often has time stamps in
there to reassemble the log lines but one had to do this manually.
L2 domains are not bound to local networks anymore, thanks to vxlan etc.
If you are in a controlled environment and you do know your software
that runs perfectly well, certainly, no doubt, UDP hashing can be
enabled. I would argue we can't do that generally.
> Btw, TCP has exactly the same problem in this regard that UDP has with
> regard to fragmentation. The reason that TCP isn't considered an issue
> is the assumption that TCP will do PMTU discovery and set DF (I would
> bet that devices don't actually check for DF and vendors don't test
> when it's not set!).
I don't know.
For the application the stream will appear in order at the socket level
and OoO or fragmentation won't break anything. End systems will have a
harder time reassembling the stream and thus fast paths couldn't be
used. Is that what you are referring to?
Thanks,
Hannes
^ permalink raw reply
* Re: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: Stephen Hemminger @ 2017-09-04 18:25 UTC (permalink / raw)
To: Phil Sutter; +Cc: David Laight, netdev@vger.kernel.org
In-Reply-To: <20170904150015.GB30364@orbyte.nwl.cc>
On Mon, 4 Sep 2017 17:00:15 +0200
Phil Sutter <phil@nwl.cc> wrote:
> On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> > From: Phil Sutter
> > > Sent: 01 September 2017 17:53
> > > By making use of strncpy(), both implementations are really simple so
> > > there is no need to add libbsd as additional dependency.
> > >
> > ...
> > > +
> > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > +{
> > > + if (size) {
> > > + strncpy(dst, src, size - 1);
> > > + dst[size - 1] = '\0';
> > > + }
> > > + return strlen(src);
> > > +}
> >
> > Except that isn't really strlcpy().
> > Better would be:
> > len = strlen(src) + 1;
> > if (len <= size)
> > memcpy(dst, src, len);
> > else if (size) {
> > dst[size - 1] = 0;
> > memcpy(dst, src, size - 1);
> > }
> > return len - 1;
>
> Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> proposed version better?
>
> Thanks, Phil
Linux kernel:
size_t strlcpy(char *dest, const char *src, size_t size)
{
size_t ret = strlen(src);
if (size) {
size_t len = (ret >= size) ? size - 1 : ret;
memcpy(dest, src, len);
dest[len] = '\0';
}
return ret;
}
FreeBSD:
size_t
strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
{
const char *osrc = src;
size_t nleft = dsize;
/* Copy as many bytes as will fit. */
if (nleft != 0) {
while (--nleft != 0) {
if ((*dst++ = *src++) == '\0')
break;
}
}
/* Not enough room in dst, add NUL and traverse rest of src. */
if (nleft == 0) {
if (dsize != 0)
*dst = '\0'; /* NUL-terminate dst */
while (*src++)
;
}
return(src - osrc - 1); /* count does not include NUL */
}
They all give the same results for some basic tests.
Test FreeBSD Linux Iproute2
"",0: 0 "JUNK" 0 "JUNK" 0 "JUNK"
"",1: 0 "" 0 "" 0 ""
"",8: 0 "" 0 "" 0 ""
"foo",0: 3 "JUNK" 3 "JUNK" 3 "JUNK"
"foo",3: 3 "fo" 3 "fo" 3 "fo"
"foo",4: 3 "foo" 3 "foo" 3 "foo"
"foo",8: 3 "foo" 3 "foo" 3 "foo"
"longstring",0: 10 "JUNK" 10 "JUNK" 10 "JUNK"
"longstring",8: 10 "longstr" 10 "longstr" 10 "longstr"
^ 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