* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Kyle Mestery (kmestery) @ 2012-04-24 16:16 UTC (permalink / raw)
To: Stephen Hemminger
Cc: <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
<eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
<jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>,
<stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org>,
David Miller
In-Reply-To: <20120424091317.08953fd2-We1ePj4FEcvRI77zikRAJc56i+j3xesD0e7PPNI6Mm0@public.gmane.org>
On Apr 24, 2012, at 11:13 AM, Stephen Hemminger wrote:
> On Tue, 24 Apr 2012 16:02:41 +0000
> "Kyle Mestery (kmestery)" <kmestery-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Apr 23, 2012, at 9:25 PM, Simon Horman wrote:
>>> On Mon, Apr 23, 2012 at 03:59:24PM -0700, Jesse Gross wrote:
>>>> On Mon, Apr 23, 2012 at 3:32 PM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>>>>> On Mon, Apr 23, 2012 at 02:38:07PM -0700, Jesse Gross wrote:
>>>>>> On Mon, Apr 23, 2012 at 2:08 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>>>>>>> From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
>>>>>>> Date: Mon, 23 Apr 2012 13:53:42 -0700
>>>>>>>
>>>>>>>> On Mon, Apr 23, 2012 at 1:13 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>>>>>>>>> From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
>>>>>>>>> Date: Mon, 23 Apr 2012 13:08:49 -0700
>>>>>>>>>
>>>>>>>>>> Assuming that the TCP stack generates large TSO frames on transmit
>>>>>>>>>> (which could be the local stack; something sent by a VM; or packets
>>>>>>>>>> received, coalesced by GRO and then encapsulated by STT) then you can
>>>>>>>>>> just prepend the STT header (possibly slightly adjusting things like
>>>>>>>>>> requested MSS, number of segments, etc. slightly). After that it's
>>>>>>>>>> possible to just output the resulting frame through the IP stack like
>>>>>>>>>> all tunnels do today.
>>>>>>>>>
>>>>>>>>> Which seems to potentially suggest a stronger intergration of the STT
>>>>>>>>> tunnel transmit path into our IP stack rather than the approach Simon
>>>>>>>>> is taking
>>>>>>>>
>>>>>>>> Did you have something in mind?
>>>>>>>
>>>>>>> A normal bonafide tunnel netdevice driver like GRE instead of the
>>>>>>> openvswitch approach Simon is using.
>>>>>>
>>>>>> Ahh, yes, that I agree with. Independent of this, there's work being
>>>>>> done to make it so that OVS can use the normal in-tree tunneling code
>>>>>> and not need its own. Once that's done I expect that STT will follow
>>>>>> the same model.
>>>>>
>>>>> Hi Jesse,
>>>>>
>>>>> I am wondering how firm the plans to on allowing OVS to use in-tree tunnel
>>>>> code are. I'm happy to move my efforts over to an in-tree STT implementation
>>>>> but ultimately I would like to get STT running in conjunction with OVS.
>>>>
>>>> I would say that it's a firm goal but the implementation probably
>>>> still has a ways to go. Kyle Mestery (CC'ed) has volunteered to work
>>>> on this in support of adding VXLAN, which needs some additional
>>>> flexibility that this approach would also provide. You might want to
>>>> talk to him to see if there are ways that you guys can work together
>>>> on it if you are interested. Having better integration with upstream
>>>> tunneling is definitely a step that OVS needs to make and sooner would
>>>> be better than later.
>>>
>>> Hi Jesse, Hi Kyle,
>>>
>>> that sounds like an excellent plan.
>>>
>>> Kyle, do you have any thoughts on how we might best work together on this?
>>> Perhaps there are some patches floating around that I could take a look at?
>>>
>>
>> Hi Simon:
>>
>> The VXLAN work has been slow going for me at this point. What I have works, but is far from complete. It's available here:
>>
>> https://github.com/mestery/ovs-vxlan/tree/vxlan
>>
>> This is based on a fairly recent version of OVS. I'm currently working to allow tunnels to be flow-based rather than port-based, as they currently exist. As Jesse may have mentioned, doing this allows us to move most tunnel state into user space. The outer header can now be part of the flow lookup and can be passed to user space, so things like multicast learning for VXLAN become possible.
>>
>> With regards to working together, ping me off-list and we can work something out, I'm very much in favor of this!
>>
>
> My use of VXVLAN was to be key based (like existing GRE), not flow based.
>
Yes, for OVS the idea is to add the tunnel key values to the flow-key in the OVS kernel module.
^ permalink raw reply
* Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
From: Christoph Lameter @ 2012-04-24 16:24 UTC (permalink / raw)
To: Glauber Costa
Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan,
kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, David Miller,
devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <4F96D1A8.6040604-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
On Tue, 24 Apr 2012, Glauber Costa wrote:
> > Would this not also be a good case to introduce static branching?
> >
> > number_of_cpusets is used to avoid going through unnecessary processing
> > should there be no cpusets in use.
>
> static branches comes with a set of problems themselves, so I usually prefer
> to use them only in places where we don't want to pay even a cache miss if we
> can avoid, or a function call, or anything like that - like the slub cache
> alloc as you may have seen in my kmem memcg series.
>
> It doesn't seem to be the case here.
How did you figure that? number_of_cpusets was introduced exactly because
the functions are used in places where we do not pay the cost of calling
__cpuset_node_allowed_soft/hardwall. Have a look at these. They may take
locks etc etc in critical allocation paths
^ permalink raw reply
* Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
From: Glauber Costa @ 2012-04-24 16:30 UTC (permalink / raw)
To: Christoph Lameter
Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan,
kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, David Miller,
devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <alpine.DEB.2.00.1204241120130.26005-sBS69tsa9Uj/9pzu0YdTqQ@public.gmane.org>
On 04/24/2012 01:24 PM, Christoph Lameter wrote:
> On Tue, 24 Apr 2012, Glauber Costa wrote:
>
>>> Would this not also be a good case to introduce static branching?
>>>
>>> number_of_cpusets is used to avoid going through unnecessary processing
>>> should there be no cpusets in use.
>>
>> static branches comes with a set of problems themselves, so I usually prefer
>> to use them only in places where we don't want to pay even a cache miss if we
>> can avoid, or a function call, or anything like that - like the slub cache
>> alloc as you may have seen in my kmem memcg series.
>>
>> It doesn't seem to be the case here.
>
> How did you figure that? number_of_cpusets was introduced exactly because
> the functions are used in places where we do not pay the cost of calling
> __cpuset_node_allowed_soft/hardwall. Have a look at these. They may take
> locks etc etc in critical allocation paths
I am not arguing that.
You want to avoid the cost of processing a function, that's fair.
(Note that by "function call cost" I don't mean the cost of processing a
function, but the cost of a (potentially empty) function call.)
The real question is: Are you okay with the cost of a branch + a global
variable (which is almost read only) fetch?
The test of a global variable can - and do as of right now - avoid all
the expensive operations like locking, sleeping, etc, and if you don't
need to squeeze every nanosecond you can, they are often simpler - and
therefore better - than static branching.
Just to mention one point I am coming across these days - that initiated
all this: static patching holds the cpu_hotplug.lock. So it can't be
called if you hold any lock that has been already held under the
cpu_hotplug.lock. This will probably mean any lock the cpuset cgroup
needs to take, because it is called - and to do a lot of things - from
the cpu hotplug handler, that holds the cpu_hotplug.lock.
So if if were a case of simple static branch usage, I am not opposed to
it. But I foresee it getting so complicated, that a global variable
seems to do the job we need just fine.
^ permalink raw reply
* Re: [PATCH net-next 2/3] Add capability to retrieve plug-in module EEPROM
From: Ben Hutchings @ 2012-04-24 17:29 UTC (permalink / raw)
To: Stuart Hodgson
Cc: netdev, bruce.w.allan, decot, alexander.h.duyck, davem,
linux-kernel
In-Reply-To: <4F9031CE.1060702@solarflare.com>
On Thu, 2012-04-19 at 16:39 +0100, Stuart Hodgson wrote:
> Provides a new struct ethtool_modinfo that will return the
> type and size of plug-in module eeprom (such as SFP+) for parsing
> by userland program.
>
> The second provides the API to get the raw eeprom information
> using the existing ethtool_eeprom structture to return the data
>
> Signed-off-by: Stuart Hodgson <smhodgson@solarflare.com>
> ---
> include/linux/ethtool.h | 33 +++++++++++++++++++++++++++++++++
> net/core/ethtool.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 89d68d8..f6500f3 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -137,6 +137,23 @@ struct ethtool_eeprom {
> };
>
> /**
> + * struct ethtool_modinfo - plugin module eeprom information
> + * @cmd: ETHTOOL_GMODULEINFO
> + * @type: Standard the module information conforms to SFF_xxxx
You renamed the type codes to begin with 'ETH_MODULE_SFF_', so this line
needs to be updated too.
> + * @eeprom_len: Length of the eeprom
> + *
> + * This structure is used to return the information to
> + * properly size memory for a subsequent call to ETHTOOL_GMODULEEEPROM
[...]
The type code is also essential in order to parse the information.
Also, a minor nitpick: you should put a '%' in front of the names of
constants so they can be pretty-printed properly.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Eric Dumazet @ 2012-04-24 17:37 UTC (permalink / raw)
To: Tore Anderson, David Miller; +Cc: netdev, Maciej Żenczykowski, Tom Herbert
From: Eric Dumazet <edumazet@google.com>
Quoting Tore Anderson from :
https://bugzilla.kernel.org/show_bug.cgi?id=42572
When RTAX_FEATURE_ALLFRAG is set on a route, the effective TCP segment
size does not take into account the size of the IPv6 Fragmentation
header that needs to be included in outbound packets, causing every
transmitted TCP segment to be fragmented across two IPv6 packets, the
latter of which will only contain 8 bytes of actual payload.
RTAX_FEATURE_ALLFRAG is typically set on a route in response to
receving a ICMPv6 Packet Too Big message indicating a Path MTU of less
than 1280 bytes. 1280 bytes is the minimum IPv6 MTU, however ICMPv6
PTBs with MTU < 1280 are still valid, in particular when an IPv6
packet is sent to an IPv4 destination through a stateless translator.
Any ICMPv4 Need To Fragment packets originated from the IPv4 part of
the path will be translated to ICMPv6 PTB which may then indicate an
MTU of less than 1280.
The Linux kernel refuses to reduce the effective MTU to anything below
1280 bytes, instead it sets it to exactly 1280 bytes, and
RTAX_FEATURE_ALLFRAG is also set. However, the TCP segment size appears
to be set to 1240 bytes (1280 Path MTU - 40 bytes of IPv6 header),
instead of 1232 (additionally taking into account the 8 bytes required
by the IPv6 Fragmentation extension header).
This in turn results in rather inefficient transmission, as every
transmitted TCP segment now is split in two fragments containing
1232+8 bytes of payload.
After this patch, all the outgoing packets that includes a
Fragmentation header all are "atomic" or "non-fragmented" fragments,
i.e., they both have Offset=0 and More Fragments=0.
With help from David S. Miller
Reported-by: Tore Anderson <tore@fud.no>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Maciej Żenczykowski <maze@google.com>
Cc: Tom Herbert <therbert@google.com>
Tested-by: Tore Anderson <tore@fud.no>
---
include/net/inet_connection_sock.h | 1 +
include/net/tcp.h | 4 ++--
net/ipv4/tcp_output.c | 19 +++++++++++++++++--
net/ipv6/tcp_ipv6.c | 1 +
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 46c9e2c..7d83f90 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -45,6 +45,7 @@ struct inet_connection_sock_af_ops {
struct dst_entry *dst);
struct inet_peer *(*get_peer)(struct sock *sk, bool *release_it);
u16 net_header_len;
+ u16 net_frag_header_len;
u16 sockaddr_len;
int (*setsockopt)(struct sock *sk, int level, int optname,
char __user *optval, unsigned int optlen);
diff --git a/include/net/tcp.h b/include/net/tcp.h
index fc880e9..0fb84de 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -544,8 +544,8 @@ extern int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
extern void tcp_initialize_rcv_mss(struct sock *sk);
-extern int tcp_mtu_to_mss(const struct sock *sk, int pmtu);
-extern int tcp_mss_to_mtu(const struct sock *sk, int mss);
+extern int tcp_mtu_to_mss(struct sock *sk, int pmtu);
+extern int tcp_mss_to_mtu(struct sock *sk, int mss);
extern void tcp_mtup_init(struct sock *sk);
extern void tcp_valid_rtt_meas(struct sock *sk, u32 seq_rtt);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 7b7cf38..834e89f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1150,7 +1150,7 @@ int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
}
/* Calculate MSS. Not accounting for SACKs here. */
-int tcp_mtu_to_mss(const struct sock *sk, int pmtu)
+int tcp_mtu_to_mss(struct sock *sk, int pmtu)
{
const struct tcp_sock *tp = tcp_sk(sk);
const struct inet_connection_sock *icsk = inet_csk(sk);
@@ -1161,6 +1161,14 @@ int tcp_mtu_to_mss(const struct sock *sk, int pmtu)
*/
mss_now = pmtu - icsk->icsk_af_ops->net_header_len - sizeof(struct tcphdr);
+ /* IPv6 adds a frag_hdr in case RTAX_FEATURE_ALLFRAG is set */
+ if (icsk->icsk_af_ops->net_frag_header_len) {
+ const struct dst_entry *dst = __sk_dst_get(sk);
+
+ if (dst && dst_allfrag(dst))
+ mss_now -= icsk->icsk_af_ops->net_frag_header_len;
+ }
+
/* Clamp it (mss_clamp does not include tcp options) */
if (mss_now > tp->rx_opt.mss_clamp)
mss_now = tp->rx_opt.mss_clamp;
@@ -1179,7 +1187,7 @@ int tcp_mtu_to_mss(const struct sock *sk, int pmtu)
}
/* Inverse of above */
-int tcp_mss_to_mtu(const struct sock *sk, int mss)
+int tcp_mss_to_mtu(struct sock *sk, int mss)
{
const struct tcp_sock *tp = tcp_sk(sk);
const struct inet_connection_sock *icsk = inet_csk(sk);
@@ -1190,6 +1198,13 @@ int tcp_mss_to_mtu(const struct sock *sk, int mss)
icsk->icsk_ext_hdr_len +
icsk->icsk_af_ops->net_header_len;
+ /* IPv6 adds a frag_hdr in case RTAX_FEATURE_ALLFRAG is set */
+ if (icsk->icsk_af_ops->net_frag_header_len) {
+ const struct dst_entry *dst = __sk_dst_get(sk);
+
+ if (dst && dst_allfrag(dst))
+ mtu += icsk->icsk_af_ops->net_frag_header_len;
+ }
return mtu;
}
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index cdbf292..57b2109 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1778,6 +1778,7 @@ static const struct inet_connection_sock_af_ops ipv6_specific = {
.syn_recv_sock = tcp_v6_syn_recv_sock,
.get_peer = tcp_v6_get_peer,
.net_header_len = sizeof(struct ipv6hdr),
+ .net_frag_header_len = sizeof(struct frag_hdr),
.setsockopt = ipv6_setsockopt,
.getsockopt = ipv6_getsockopt,
.addr2sockaddr = inet6_csk_addr2sockaddr,
^ permalink raw reply related
* Re: [PATCH net-next 3/3] Add capability to retrieve plug-in module EEPROM
From: Ben Hutchings @ 2012-04-24 17:43 UTC (permalink / raw)
To: Stuart Hodgson
Cc: netdev, bruce.w.allan, decot, alexander.h.duyck, davem,
linux-kernel
In-Reply-To: <4F9031C7.9070206@solarflare.com>
On Thu, 2012-04-19 at 16:39 +0100, Stuart Hodgson wrote:
> Currently allows for SFP+ eeprom to be returned using the ethtool API.
> This can be extended in future to handle different eeprom formats
> and sizes
[...]
> --- a/drivers/net/ethernet/sfc/mcdi_phy.c
> +++ b/drivers/net/ethernet/sfc/mcdi_phy.c
> @@ -304,6 +304,26 @@ static u32 mcdi_to_ethtool_media(u32 media)
> }
> }
>
> +static u32 mcdi_to_module_eeprom_len(u32 media)
> +{
> + switch (media) {
> + case MC_CMD_MEDIA_SFP_PLUS:
> + return ETH_MODULE_SFF_8079_LEN;
> + default:
> + return 0;
> + }
> +}
> +
> +static u32 mcdi_to_module_eeprom_type(u32 media)
> +{
> + switch (media) {
> + case MC_CMD_MEDIA_SFP_PLUS:
> + return ETH_MODULE_SFF_8079;
> + default:
> + return 0;
> + }
> +}
These functions ended up unused, so don't add them. You should have got
a compiler warning about this...
> static int efx_mcdi_phy_probe(struct efx_nic *efx)
> {
> struct efx_mcdi_phy_data *phy_data;
> @@ -739,6 +759,95 @@ static const char *efx_mcdi_phy_test_name(struct efx_nic *efx,
> return NULL;
> }
>
> +#define SFP_PAGE_SIZE 128
> +#define SFP_NUM_PAGES 2
> +static int efx_mcdi_phy_get_module_eeprom(struct efx_nic *efx,
> + struct ethtool_eeprom *ee, u8 *data)
> +{
> + u8 outbuf[MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMAX];
> + u8 inbuf[MC_CMD_GET_PHY_MEDIA_INFO_IN_LEN];
> + size_t outlen;
> + int rc;
> + unsigned int payload_len;
> + unsigned int copied = 0;
> + unsigned int space_remaining = ee->len;
> + unsigned int page;
> + unsigned int page_off;
> + unsigned int to_copy;
> + u8 *user_data = data;
> +
> + if (ee->offset > (SFP_PAGE_SIZE * SFP_NUM_PAGES))
> + return -EINVAL;
Please also add this sanity-check:
BUILD_BUG_ON(SFP_PAGE_SIZE * SFP_NUM_PAGES != ETH_MODULE_SFF_8079_LEN);
> + page_off = (ee->offset % SFP_PAGE_SIZE);
> + page = (ee->offset > SFP_PAGE_SIZE) ? 1 : 0;
Off-by-one error; the comparison should be >=. Why not use the
straightforward calculation:
page = ee->offset / SFP_PAGE_SIZE;
[...]
> +static int efx_mcdi_phy_get_module_info(struct efx_nic *efx,
> + struct ethtool_modinfo *modinfo)
> +{
> + /* This will return a length of the eeprom
> + * type of the module that was detected during the probe,
> + * if not modules inserted then phy_data will be NULL */
> + struct efx_mcdi_phy_data *phy_cfg;
> +
> + phy_cfg = efx->phy_data;
> + modinfo->eeprom_len = 0;
> + modinfo->type = 0;
> +
> + switch (phy_cfg->media) {
> + case MC_CMD_MEDIA_SFP_PLUS:
> + modinfo->type = ETH_MODULE_SFF_8079;
> + modinfo->eeprom_len = ETH_MODULE_SFF_8079_LEN;
> + break;
> + default:
The default case should return -EOPNOTSUPP, just as if we didn't
implement this operation at all. And there's no need to set
modinfo->{type,eeprom_len} to 0 in that case.
Ben.
> + break;
> + }
> +
> + return 0;
> +}
[...]
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add capability to retrieve plug-in module EEPROM
From: Ben Hutchings @ 2012-04-24 17:44 UTC (permalink / raw)
To: David Miller
Cc: smhodgson, netdev, bruce.w.allan, decot, alexander.h.duyck,
linux-kernel
In-Reply-To: <20120423.172848.1733968863825393980.davem@davemloft.net>
On Mon, 2012-04-23 at 17:28 -0400, David Miller wrote:
> You can't just submit three seperate patches each with the same exact
> Subject line.
>
> Otherwise someone scanning the commit headers can't figure out what
> is different in each of these changes.
>
> There also is no signoff from Ben for patches #2 or #3, did he review
> them? If so, why didn't he ACK or sign off on it? If not, why not?
Sorry, I've been busy with another project. I'll reply to Stuart's
patches faster next round.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
From: Christoph Lameter @ 2012-04-24 18:27 UTC (permalink / raw)
To: Glauber Costa
Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan,
kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, David Miller,
devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <4F96D50D.4020804-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
On Tue, 24 Apr 2012, Glauber Costa wrote:
> > > It doesn't seem to be the case here.
> >
> > How did you figure that? number_of_cpusets was introduced exactly because
> > the functions are used in places where we do not pay the cost of calling
> > __cpuset_node_allowed_soft/hardwall. Have a look at these. They may take
> > locks etc etc in critical allocation paths
> I am not arguing that.
>
> You want to avoid the cost of processing a function, that's fair.
> (Note that by "function call cost" I don't mean the cost of processing a
> function, but the cost of a (potentially empty) function call.)
> The real question is: Are you okay with the cost of a branch + a global
> variable (which is almost read only) fetch?
No and that is why the static branching comes in. It takes away the global
read of the number_of_cpusets variable in the critical paths.
> The test of a global variable can - and do as of right now - avoid all the
> expensive operations like locking, sleeping, etc, and if you don't need to
> squeeze every nanosecond you can, they are often simpler - and therefore
> better - than static branching.
Better than static branching? This is in critical VM functions and
reducing the cache footprint there is good for everyone.
> Just to mention one point I am coming across these days - that initiated all
> this: static patching holds the cpu_hotplug.lock. So it can't be called if you
> hold any lock that has been already held under the cpu_hotplug.lock. This will
> probably mean any lock the cpuset cgroup needs to take, because it is called -
> and to do a lot of things - from the cpu hotplug handler, that holds the
> cpu_hotplug.lock.
Transitions from one to two cpusets are rare and are only done when a
cpuset is created in the /dev/cpuset hierachy). You could move the
code modification outside of locks or defer action into an event
thread if there are locks in the way.
^ permalink raw reply
* Re: restoring IP multicast addresses when restarting the interface.
From: Flavio Leitner @ 2012-04-24 18:30 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, David Miller
In-Reply-To: <20120423121425.GA29883@gondor.apana.org.au>
On Mon, 23 Apr 2012 22:14:25 +1000
Herbert Xu <herbert@gondor.hengli.com.au> wrote:
> On Fri, Apr 20, 2012 at 09:25:33PM -0300, Flavio Leitner wrote:
> >
> > Although the new behavior seems nice and save some user space
> > work, I think it was unintentional and likely to be a bug.
> >
> > What you guys think?
>
> Are you talking about multicast subscriptions on the interface?
Yes.
> I don't see why they should disappear when the interface goes
> down and then comes back up since these ultimately come from
> application sockets which continue to exist after a down/up.
Yeah, but that's not how things used to work before, so my
question is if the kernel should be responsible for keeping
the subscription or the application.
If the admin puts down the interface and remove the module,
for instance, then the multicast subscription is gone.
Should the application monitor for that then?
David? Any thoughts?
thanks,
fbl
^ permalink raw reply
* Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove
From: Andy Fleming @ 2012-04-24 19:05 UTC (permalink / raw)
To: Jonas Gorski
Cc: mbizon, Andy Fleming, netdev, Florian Fainelli, Eric Dumazet,
David S. Miller
In-Reply-To: <CAOiHx=nG_v11LWoc39236aPyFOjOPXJQz6BVNSVqmXhv_jKDVA@mail.gmail.com>
On Sun, Apr 22, 2012 at 6:31 AM, Jonas Gorski <jonas.gorski@gmail.com> wrote:
> On 19 April 2012 18:17, Maxime Bizon <mbizon@freebox.fr> wrote:
>>
>> On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote:
>>
>>> Yes, but none of the ethtool functions cause register writes in the
>>> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All
>>> they do is modify the phy_device's settings.
>>
>> unless I'm mistaken:
>>
>> phy_ethtool_sset() => phy_start_aneg()
>>
>> will kick the state machine even when state is PHY_READY
>
> Hmm. I see what you mean. I wonder if it is intended that you can do
> that without having phy_start() called first.
>
> @Andy, can you perhaps shed some light on this? How are ethernet
> drivers supposed to behave/when should they call
> phy_connect()/phy_start()? Currently most drivers call phy_connect()
> in their _probe(), and phy_start() in _open(), so many seem to have
> the issue that the phy state machine is in PHY_READY after _probe(),
> and can be kicked into running through ethtool even if the interface
> is down.
>
> This problem goes away after the first ifup/ifdown cycle, since the
> phy state machine is then in PHY_HALTED, which gets properly caught in
> phy_start_aneg().
>
> To me it looks like phy_start_aneg() should check for some more
> states, as it currently would also overwrite a PHY_STARTING or
> PHY_PENDING state, which looks definitely wrong to me.
Ugh, it looks like much has gone wrong with the state machine
(possibly from when I wrote it). I'm thinking that what we should
probably do is eliminate PHY_UP and PHY_READY, and have the PHY come
up to PHY_HALTED. For some reason, PHY_RESUMING always enables
interrupts, even if phydev->irq is in POLL mode, so that should be
fixed.
Other than that, it looks like PHY_RESUMING should work in the place
of PHY_UP, and PHY_HALTED should be the same as PHY_READY...
Andy
^ permalink raw reply
* Hello
From: Abegail Benson @ 2012-04-24 19:40 UTC (permalink / raw)
Hello..
my name is Abegail i seek for a honest
partner for friendship i hope you
don't mind
if you don't mind please reply me i will
like to hear from you ok so i can tell
you more about myself and we
can exchange picture too and share
some other things with love
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-24 19:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Tore Anderson, David Miller, netdev, Tom Herbert
In-Reply-To: <1335289058.5205.165.camel@edumazet-glaptop>
Why do we refuse to set ipv6 mtu's below 1280?
how is what we do any better?
On Tue, Apr 24, 2012 at 10:37 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Quoting Tore Anderson from :
> https://bugzilla.kernel.org/show_bug.cgi?id=42572
>
> When RTAX_FEATURE_ALLFRAG is set on a route, the effective TCP segment
> size does not take into account the size of the IPv6 Fragmentation
> header that needs to be included in outbound packets, causing every
> transmitted TCP segment to be fragmented across two IPv6 packets, the
> latter of which will only contain 8 bytes of actual payload.
>
> RTAX_FEATURE_ALLFRAG is typically set on a route in response to
> receving a ICMPv6 Packet Too Big message indicating a Path MTU of less
> than 1280 bytes. 1280 bytes is the minimum IPv6 MTU, however ICMPv6
> PTBs with MTU < 1280 are still valid, in particular when an IPv6
> packet is sent to an IPv4 destination through a stateless translator.
> Any ICMPv4 Need To Fragment packets originated from the IPv4 part of
> the path will be translated to ICMPv6 PTB which may then indicate an
> MTU of less than 1280.
>
> The Linux kernel refuses to reduce the effective MTU to anything below
> 1280 bytes, instead it sets it to exactly 1280 bytes, and
> RTAX_FEATURE_ALLFRAG is also set. However, the TCP segment size appears
> to be set to 1240 bytes (1280 Path MTU - 40 bytes of IPv6 header),
> instead of 1232 (additionally taking into account the 8 bytes required
> by the IPv6 Fragmentation extension header).
>
> This in turn results in rather inefficient transmission, as every
> transmitted TCP segment now is split in two fragments containing
> 1232+8 bytes of payload.
>
> After this patch, all the outgoing packets that includes a
> Fragmentation header all are "atomic" or "non-fragmented" fragments,
> i.e., they both have Offset=0 and More Fragments=0.
>
> With help from David S. Miller
>
> Reported-by: Tore Anderson <tore@fud.no>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Maciej Żenczykowski <maze@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Tested-by: Tore Anderson <tore@fud.no>
> ---
> include/net/inet_connection_sock.h | 1 +
> include/net/tcp.h | 4 ++--
> net/ipv4/tcp_output.c | 19 +++++++++++++++++--
> net/ipv6/tcp_ipv6.c | 1 +
> 4 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> index 46c9e2c..7d83f90 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h
> @@ -45,6 +45,7 @@ struct inet_connection_sock_af_ops {
> struct dst_entry *dst);
> struct inet_peer *(*get_peer)(struct sock *sk, bool *release_it);
> u16 net_header_len;
> + u16 net_frag_header_len;
> u16 sockaddr_len;
> int (*setsockopt)(struct sock *sk, int level, int optname,
> char __user *optval, unsigned int optlen);
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index fc880e9..0fb84de 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -544,8 +544,8 @@ extern int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>
> extern void tcp_initialize_rcv_mss(struct sock *sk);
>
> -extern int tcp_mtu_to_mss(const struct sock *sk, int pmtu);
> -extern int tcp_mss_to_mtu(const struct sock *sk, int mss);
> +extern int tcp_mtu_to_mss(struct sock *sk, int pmtu);
> +extern int tcp_mss_to_mtu(struct sock *sk, int mss);
> extern void tcp_mtup_init(struct sock *sk);
> extern void tcp_valid_rtt_meas(struct sock *sk, u32 seq_rtt);
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 7b7cf38..834e89f 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1150,7 +1150,7 @@ int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
> }
>
> /* Calculate MSS. Not accounting for SACKs here. */
> -int tcp_mtu_to_mss(const struct sock *sk, int pmtu)
> +int tcp_mtu_to_mss(struct sock *sk, int pmtu)
> {
> const struct tcp_sock *tp = tcp_sk(sk);
> const struct inet_connection_sock *icsk = inet_csk(sk);
> @@ -1161,6 +1161,14 @@ int tcp_mtu_to_mss(const struct sock *sk, int pmtu)
> */
> mss_now = pmtu - icsk->icsk_af_ops->net_header_len - sizeof(struct tcphdr);
>
> + /* IPv6 adds a frag_hdr in case RTAX_FEATURE_ALLFRAG is set */
> + if (icsk->icsk_af_ops->net_frag_header_len) {
> + const struct dst_entry *dst = __sk_dst_get(sk);
> +
> + if (dst && dst_allfrag(dst))
> + mss_now -= icsk->icsk_af_ops->net_frag_header_len;
> + }
> +
> /* Clamp it (mss_clamp does not include tcp options) */
> if (mss_now > tp->rx_opt.mss_clamp)
> mss_now = tp->rx_opt.mss_clamp;
> @@ -1179,7 +1187,7 @@ int tcp_mtu_to_mss(const struct sock *sk, int pmtu)
> }
>
> /* Inverse of above */
> -int tcp_mss_to_mtu(const struct sock *sk, int mss)
> +int tcp_mss_to_mtu(struct sock *sk, int mss)
> {
> const struct tcp_sock *tp = tcp_sk(sk);
> const struct inet_connection_sock *icsk = inet_csk(sk);
> @@ -1190,6 +1198,13 @@ int tcp_mss_to_mtu(const struct sock *sk, int mss)
> icsk->icsk_ext_hdr_len +
> icsk->icsk_af_ops->net_header_len;
>
> + /* IPv6 adds a frag_hdr in case RTAX_FEATURE_ALLFRAG is set */
> + if (icsk->icsk_af_ops->net_frag_header_len) {
> + const struct dst_entry *dst = __sk_dst_get(sk);
> +
> + if (dst && dst_allfrag(dst))
> + mtu += icsk->icsk_af_ops->net_frag_header_len;
> + }
> return mtu;
> }
>
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index cdbf292..57b2109 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1778,6 +1778,7 @@ static const struct inet_connection_sock_af_ops ipv6_specific = {
> .syn_recv_sock = tcp_v6_syn_recv_sock,
> .get_peer = tcp_v6_get_peer,
> .net_header_len = sizeof(struct ipv6hdr),
> + .net_frag_header_len = sizeof(struct frag_hdr),
> .setsockopt = ipv6_setsockopt,
> .getsockopt = ipv6_getsockopt,
> .addr2sockaddr = inet6_csk_addr2sockaddr,
>
>
--
Maciej A. Żenczykowski
Kernel Networking Developer @ Google
1600 Amphitheatre Parkway, Mountain View, CA 94043
tel: +1 (650) 253-0062
^ permalink raw reply
* Re: BCM5720
From: Matt Carlson @ 2012-04-24 20:06 UTC (permalink / raw)
To: Mahesh Bandewar; +Cc: Matt Carlson, Michael Chan, linux-netdev
In-Reply-To: <CAF2d9jgpfV4rod95LdhicyO5bD_9Umsnhev+UwaT03EMyJGxug@mail.gmail.com>
On Tue, Apr 24, 2012 at 01:00:12PM -0700, Mahesh Bandewar wrote:
> Hi,
>
> The PCI device ID for this was removed sometime in the past. Was there any
> reason behind this (am I missing anything)?
The device ID that was removed was for a preproduction device that was
never released. There now exists a new device under the same name, but
it has a different device ID. If you have a BCM5720 device, it should
work with the latest kernels.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Eric Dumazet @ 2012-04-24 20:10 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Tore Anderson, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGd0mV8RcH1zCLBg+N9sgPGbiynhOSfoUAYpcMgYsvzeNA@mail.gmail.com>
On Tue, 2012-04-24 at 12:49 -0700, Maciej Żenczykowski wrote:
> Why do we refuse to set ipv6 mtu's below 1280?
> how is what we do any better?
I guess you didnt read Tore use case.
Thats the standard : http://tools.ietf.org/html/rfc2460#section-5
In response to an IPv6 packet that is sent to an IPv4 destination
(i.e., a packet that undergoes translation from IPv6 to IPv4), the
originating IPv6 node may receive an ICMP Packet Too Big message
reporting a Next-Hop MTU less than 1280. In that case, the IPv6 node
is not required to reduce the size of subsequent packets to less than
1280, but must include a Fragment header in those packets so that the
IPv6-to-IPv4 translating router can obtain a suitable Identification
value to use in resulting IPv4 fragments. Note that this means the
payload may have to be reduced to 1232 octets (1280 minus 40 for the
IPv6 header and 8 for the Fragment header), and smaller still if
additional extension headers are used.
^ permalink raw reply
* [PATCH net-next] ipv6: call consume_skb() in frag/reassembly
From: Eric Dumazet @ 2012-04-24 20:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
Some kfree_skb() calls should be replaced by consume_skb() to avoid
drop_monitor/dropwatch false positives.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv6/ip6_output.c | 4 ++--
net/ipv6/reassembly.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index b7ca461..b347062 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -210,7 +210,7 @@ int ip6_xmit(struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
kfree_skb(skb);
return -ENOBUFS;
}
- kfree_skb(skb);
+ consume_skb(skb);
skb = skb2;
skb_set_owner_w(skb, sk);
}
@@ -889,7 +889,7 @@ slow_path:
}
IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
IPSTATS_MIB_FRAGOKS);
- kfree_skb(skb);
+ consume_skb(skb);
return err;
fail:
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 36e04cf..54c5d2b 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -433,7 +433,7 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
skb_morph(head, fq->q.fragments);
head->next = fq->q.fragments->next;
- kfree_skb(fq->q.fragments);
+ consume_skb(fq->q.fragments);
fq->q.fragments = head;
}
^ permalink raw reply related
* Re: restoring IP multicast addresses when restarting the interface.
From: David Miller @ 2012-04-24 21:00 UTC (permalink / raw)
To: fbl; +Cc: herbert, netdev
In-Reply-To: <20120424153023.0913956e@asterix.rh>
From: Flavio Leitner <fbl@redhat.com>
Date: Tue, 24 Apr 2012 15:30:23 -0300
> On Mon, 23 Apr 2012 22:14:25 +1000
> Herbert Xu <herbert@gondor.hengli.com.au> wrote:
>
>> On Fri, Apr 20, 2012 at 09:25:33PM -0300, Flavio Leitner wrote:
>> >
>> > Although the new behavior seems nice and save some user space
>> > work, I think it was unintentional and likely to be a bug.
>> >
>> > What you guys think?
>>
>> Are you talking about multicast subscriptions on the interface?
>
> Yes.
>
>> I don't see why they should disappear when the interface goes
>> down and then comes back up since these ultimately come from
>> application sockets which continue to exist after a down/up.
>
> Yeah, but that's not how things used to work before, so my
> question is if the kernel should be responsible for keeping
> the subscription or the application.
>
> If the admin puts down the interface and remove the module,
> for instance, then the multicast subscription is gone.
> Should the application monitor for that then?
>
> David? Any thoughts?
David Stevens at IBM and Herbert at the current multicast
experts, so I will defer to them.
^ permalink raw reply
* Re: restoring IP multicast addresses when restarting the interface.
From: David Stevens @ 2012-04-24 21:28 UTC (permalink / raw)
To: David Miller; +Cc: fbl, herbert, netdev, netdev-owner
In-Reply-To: <20120424.170004.1811627706738841106.davem@davemloft.net>
netdev-owner@vger.kernel.org wrote on 04/24/2012 02:00:04 PM:
> From: David Miller <davem@davemloft.net>
> >> I don't see why they should disappear when the interface goes
> >> down and then comes back up since these ultimately come from
> >> application sockets which continue to exist after a down/up.
> >
> > Yeah, but that's not how things used to work before, so my
> > question is if the kernel should be responsible for keeping
> > the subscription or the application.
> >
> > If the admin puts down the interface and remove the module,
> > for instance, then the multicast subscription is gone.
> > Should the application monitor for that then?
> >
> > David? Any thoughts?
>
> David Stevens at IBM and Herbert at the current multicast
> experts, so I will defer to them.
I think restoring the hardware multicast filter on device up
is useful, desirable and not a bug. :-)
+-DLS
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-24 21:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Tore Anderson, David Miller, netdev, Tom Herbert
In-Reply-To: <1335298228.5205.168.camel@edumazet-glaptop>
On Tue, Apr 24, 2012 at 1:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2012-04-24 at 12:49 -0700, Maciej Żenczykowski wrote:
>> Why do we refuse to set ipv6 mtu's below 1280?
>> how is what we do any better?
>
> I guess you didnt read Tore use case.
>
> Thats the standard : http://tools.ietf.org/html/rfc2460#section-5
>
> In response to an IPv6 packet that is sent to an IPv4 destination
> (i.e., a packet that undergoes translation from IPv6 to IPv4), the
> originating IPv6 node may receive an ICMP Packet Too Big message
> reporting a Next-Hop MTU less than 1280. In that case, the IPv6 node
> is not required to reduce the size of subsequent packets to less than
... is not required to reduce...
but doesn't that mean it _may_ reduce anyway?
and thus make life easier on the guy who will have to fragment (and
thus he won't have to fragment).
[of course you'd still need to lose 8 bytes for the frag header...]
> 1280, but must include a Fragment header in those packets so that the
> IPv6-to-IPv4 translating router can obtain a suitable Identification
> value to use in resulting IPv4 fragments. Note that this means the
> payload may have to be reduced to 1232 octets (1280 minus 40 for the
> IPv6 header and 8 for the Fragment header), and smaller still if
> additional extension headers are used.
I just don't see what not decreasing mtu below 1280 buys us.
^ permalink raw reply
* Re: My e1000e GBE card is eating all port 623 pks
From: Jesse Brandeburg @ 2012-04-24 21:51 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: netdev
In-Reply-To: <OF8F83D7F0.387F6CCE-ONC12579E7.0049B885-C12579E7.004A4356@transmode.se>
On Sat, 21 Apr 2012 15:31:09 +0200
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Looks like port 623 is some mgmt protocol and our e1000e boards are eating these
> pkgs and this trips NIS, finger and yptest hangs for a long time before timing out and
> moving on.
>
> Is there a way to tell the network stack not to eat port 623 pkgs or
> have NIS not to use port 623?
I think you might be looking for something like portreserve
(see man portreserve)
Jesse
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-24 21:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Tore Anderson, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGfhc+4FjB152r58AP66oZQmj0oY-Zh=9qNn9OABCXPwbw@mail.gmail.com>
> I just don't see what not decreasing mtu below 1280 buys us.
In particular it seems like it would be a good idea to drop tcp mss...
^ permalink raw reply
* Message
From: Timothy Hull @ 2012-04-24 21:58 UTC (permalink / raw)
In-Reply-To: <133462921.3194601335304615453.JavaMail.root@zim-store03.web.westnet.com.au>
[-- Attachment #1: Type: text/plain, Size: 84 bytes --]
Attached in a PDF file are details of a business proposal.
Regards,
Timothy Hull
[-- Attachment #2: Partner.pdf --]
[-- Type: application/pdf, Size: 95985 bytes --]
^ permalink raw reply
* Re: restoring IP multicast addresses when restarting the interface.
From: Flavio Leitner @ 2012-04-24 22:19 UTC (permalink / raw)
To: David Stevens; +Cc: David Miller, herbert, netdev
In-Reply-To: <OFB8B0B17A.86602877-ON882579EA.00741868-882579EA.0075F72B@us.ibm.com>
On Tue, 24 Apr 2012 14:28:33 -0700
David Stevens <dlstevens@us.ibm.com> wrote:
> netdev-owner@vger.kernel.org wrote on 04/24/2012 02:00:04 PM:
>
> > From: David Miller <davem@davemloft.net>
>
> > >> I don't see why they should disappear when the interface goes
> > >> down and then comes back up since these ultimately come from
> > >> application sockets which continue to exist after a down/up.
> > >
> > > Yeah, but that's not how things used to work before, so my
> > > question is if the kernel should be responsible for keeping
> > > the subscription or the application.
> > >
> > > If the admin puts down the interface and remove the module,
> > > for instance, then the multicast subscription is gone.
> > > Should the application monitor for that then?
> > >
> > > David? Any thoughts?
> >
> > David Stevens at IBM and Herbert at the current multicast
> > experts, so I will defer to them.
>
> I think restoring the hardware multicast filter on device up
> is useful, desirable and not a bug. :-)
Alright, thanks everyone!
fbl
^ permalink raw reply
* Re: [PATCH V2 1/2] bonding support for IPv6 transmit hashing
From: Jay Vosburgh @ 2012-04-24 22:51 UTC (permalink / raw)
To: John; +Cc: netdev
In-Reply-To: <4F8E0A8D.9080803@8192.net>
John <linux@8192.net> wrote:
In principle, I think this is a good idea, but the patch has
some (mostly style) issues, and does not apply to net-next (you don't
say what tree it is based on).
Can you address the comments noted below, and repost against the
current net-next?
>--- a/drivers/net/bonding/bond_main.c 2012-03-18 16:15:34.000000000 -0700
>+++ b/drivers/net/bonding/bond_main.c 2012-04-14 20:23:26.000000000 -0700
>@@ -3352,56 +3352,87 @@
> /*---------------------------- Hashing Policies -----------------------------*/
>
> /*
>+ * Hash for the output device based upon layer 2 data
>+ */
>+static int bond_xmit_hash_policy_l2(struct sk_buff *skb, int count)
>+{
>+ struct ethhdr *data = (struct ethhdr *)skb->data;
>+
>+ if (skb_headlen(skb) >= 6)
>+ return (data->h_dest[5] ^ data->h_source[5]) % count;
Can skb_headlen ever be less than 6 here? And why >= 6, anyway?
The offset of ->h_source[5] from skb->data is 11.
>+
>+ return 0;
>+}
>+
>+/*
> * Hash for the output device based upon layer 2 and layer 3 data. If
>- * the packet is not IP mimic bond_xmit_hash_policy_l2()
>+ * the packet is not IP, fall back on bond_xmit_hash_policy_l2()
> */
> static int bond_xmit_hash_policy_l23(struct sk_buff *skb, int count)
> {
> struct ethhdr *data = (struct ethhdr *)skb->data;
>- struct iphdr *iph = ip_hdr(skb);
>
>- if (skb->protocol == htons(ETH_P_IP)) {
>+ if (skb->protocol == htons(ETH_P_IP) &&
>+ skb_network_header_len(skb) >= sizeof(struct iphdr)) {
>+ struct iphdr *iph = ip_hdr(skb);
> return ((ntohl(iph->saddr ^ iph->daddr) & 0xffff) ^
> (data->h_dest[5] ^ data->h_source[5])) % count;
>+ } else if (skb->protocol == htons(ETH_P_IPV6) &&
>+ skb_network_header_len(skb) >= sizeof(struct ipv6hdr)) {
>+ struct ipv6hdr *ipv6h = ipv6_hdr(skb);
>+ u32 v6hash =
I personally prefer having variables declared at the head of the
function, not inside inner blocks, but if you're going to do this, there
needs to be a blank line after the declaration.
>+ (ipv6h->saddr.s6_addr32[1] ^ ipv6h->daddr.s6_addr32[1]) ^
>+ (ipv6h->saddr.s6_addr32[2] ^ ipv6h->daddr.s6_addr32[2]) ^
>+ (ipv6h->saddr.s6_addr32[3] ^ ipv6h->daddr.s6_addr32[3]);
>+ v6hash = (v6hash >> 16) ^ (v6hash >> 8) ^ v6hash;
>+ return (v6hash ^ data->h_dest[5] ^ data->h_source[5]) % count;
> }
>
>- return (data->h_dest[5] ^ data->h_source[5]) % count;
>+ return bond_xmit_hash_policy_l2(skb, count);
> }
>
> /*
> * Hash for the output device based upon layer 3 and layer 4 data. If
> * the packet is a frag or not TCP or UDP, just use layer 3 data. If it is
>- * altogether not IP, mimic bond_xmit_hash_policy_l2()
>+ * altogether not IP, fall back on bond_xmit_hash_policy_l2()
> */
> static int bond_xmit_hash_policy_l34(struct sk_buff *skb, int count)
> {
>- struct ethhdr *data = (struct ethhdr *)skb->data;
>- struct iphdr *iph = ip_hdr(skb);
>- __be16 *layer4hdr = (__be16 *)((u32 *)iph + iph->ihl);
>- int layer4_xor = 0;
>+ u32 layer4_xor = 0;
>
> if (skb->protocol == htons(ETH_P_IP)) {
>+ struct iphdr *iph = ip_hdr(skb);
> if (!ip_is_fragment(iph) &&
Blank line after iph declaration.
>- (iph->protocol == IPPROTO_TCP ||
>- iph->protocol == IPPROTO_UDP)) {
>+ (iph->protocol == IPPROTO_TCP ||
>+ iph->protocol == IPPROTO_UDP)) {
>+ __be16 *layer4hdr = (__be16 *)((u32 *)iph + iph->ihl);
Blank line afret the layer4hdr declaration.
>+ if (iph->ihl * sizeof(u32) + sizeof(__be16) * 2 >
>+ skb_headlen(skb) - skb_network_offset(skb)) goto SHORT_HEADER;
The goto needs to be on a separate line.
> layer4_xor = ntohs((*layer4hdr ^ *(layer4hdr + 1)));
>+ } else if (skb_network_header_len(skb) < sizeof(struct iphdr)) {
>+ goto SHORT_HEADER;
> }
> return (layer4_xor ^
> ((ntohl(iph->saddr ^ iph->daddr)) & 0xffff)) % count;
>-
>+ } else if (skb->protocol == htons(ETH_P_IPV6)) {
>+ struct ipv6hdr *ipv6h = ipv6_hdr(skb);
>+ if (ipv6h->nexthdr == IPPROTO_TCP || ipv6h->nexthdr == IPPROTO_UDP) {
Blank line after ipv6h.
>+ __be16 *layer4hdrv6 = (__be16 *)((u8 *)ipv6h + sizeof(struct ipv6hdr));
>+ if (sizeof(struct ipv6hdr) + sizeof(__be16) * 2 >
>+ skb_headlen(skb) - skb_network_offset(skb)) goto SHORT_HEADER;
The goto goes on a separate line.
>+ layer4_xor = (*layer4hdrv6 ^ *(layer4hdrv6 + 1));
>+ } else if (skb_network_header_len(skb) < sizeof(struct ipv6hdr)) {
>+ goto SHORT_HEADER;
>+ }
>+ layer4_xor ^=
>+ (ipv6h->saddr.s6_addr32[1] ^ ipv6h->daddr.s6_addr32[1]) ^
>+ (ipv6h->saddr.s6_addr32[2] ^ ipv6h->daddr.s6_addr32[2]) ^
>+ (ipv6h->saddr.s6_addr32[3] ^ ipv6h->daddr.s6_addr32[3]);
>+ return ((layer4_xor >> 16) ^ (layer4_xor >> 8) ^ layer4_xor) % count;
> }
>
>- return (data->h_dest[5] ^ data->h_source[5]) % count;
>-}
>-
>-/*
>- * Hash for the output device based upon layer 2 data
>- */
>-static int bond_xmit_hash_policy_l2(struct sk_buff *skb, int count)
>-{
>- struct ethhdr *data = (struct ethhdr *)skb->data;
>-
>- return (data->h_dest[5] ^ data->h_source[5]) % count;
>+ SHORT_HEADER:
This label should be lower case, and not indented at all.
-J
>+ return bond_xmit_hash_policy_l2(skb, count);
> }
>
> /*-------------------------- Device entry points ----------------------------*/
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* [PATCH] isdn/eicon: use standard __init,__exit function markup
From: H Hartley Sweeten @ 2012-04-24 22:56 UTC (permalink / raw)
To: Linux Kernel; +Cc: netdev, mac, isdn
Remove the custom DIVA_{INIT,EXIT}_FUNCTION defines and use
the standard __init,__exit markup.
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Armin Schindler <mac@melware.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
---
diff --git a/drivers/isdn/hardware/eicon/capifunc.c b/drivers/isdn/hardware/eicon/capifunc.c
index a576f32..7a0bdbd 100644
--- a/drivers/isdn/hardware/eicon/capifunc.c
+++ b/drivers/isdn/hardware/eicon/capifunc.c
@@ -1120,7 +1120,7 @@ int fax_head_line_time(char *buffer)
/*
* init (alloc) main structures
*/
-static int DIVA_INIT_FUNCTION init_main_structs(void)
+static int __init init_main_structs(void)
{
if (!(mapped_msg = (CAPI_MSG *) diva_os_malloc(0, MAX_MSG_SIZE))) {
DBG_ERR(("init: failed alloc mapped_msg."))
@@ -1181,7 +1181,7 @@ static void do_api_remove_start(void)
/*
* init
*/
-int DIVA_INIT_FUNCTION init_capifunc(void)
+int __init init_capifunc(void)
{
diva_os_initialize_spin_lock(&api_lock, "capifunc");
memset(ControllerMap, 0, MAX_DESCRIPTORS + 1);
@@ -1209,7 +1209,7 @@ int DIVA_INIT_FUNCTION init_capifunc(void)
/*
* finit
*/
-void DIVA_EXIT_FUNCTION finit_capifunc(void)
+void __exit finit_capifunc(void)
{
do_api_remove_start();
divacapi_disconnect_didd();
diff --git a/drivers/isdn/hardware/eicon/capimain.c b/drivers/isdn/hardware/eicon/capimain.c
index eabe0fa..997d46a 100644
--- a/drivers/isdn/hardware/eicon/capimain.c
+++ b/drivers/isdn/hardware/eicon/capimain.c
@@ -118,7 +118,7 @@ void diva_os_set_controller_struct(struct capi_ctr *ctrl)
/*
* module init
*/
-static int DIVA_INIT_FUNCTION divacapi_init(void)
+static int __init divacapi_init(void)
{
char tmprev[32];
int ret = 0;
@@ -144,7 +144,7 @@ static int DIVA_INIT_FUNCTION divacapi_init(void)
/*
* module exit
*/
-static void DIVA_EXIT_FUNCTION divacapi_exit(void)
+static void __exit divacapi_exit(void)
{
finit_capifunc();
printk(KERN_INFO "%s: module unloaded.\n", DRIVERLNAME);
diff --git a/drivers/isdn/hardware/eicon/diddfunc.c b/drivers/isdn/hardware/eicon/diddfunc.c
index c4c8220..b0b23ed 100644
--- a/drivers/isdn/hardware/eicon/diddfunc.c
+++ b/drivers/isdn/hardware/eicon/diddfunc.c
@@ -47,7 +47,7 @@ static void *didd_callback(void *context, DESCRIPTOR *adapter,
/*
* connect to didd
*/
-static int DIVA_INIT_FUNCTION connect_didd(void)
+static int __init connect_didd(void)
{
int x = 0;
int dadapter = 0;
@@ -79,7 +79,7 @@ static int DIVA_INIT_FUNCTION connect_didd(void)
/*
* disconnect from didd
*/
-static void DIVA_EXIT_FUNCTION disconnect_didd(void)
+static void __exit disconnect_didd(void)
{
IDI_SYNC_REQ req;
@@ -92,7 +92,7 @@ static void DIVA_EXIT_FUNCTION disconnect_didd(void)
/*
* init
*/
-int DIVA_INIT_FUNCTION diddfunc_init(void)
+int __init diddfunc_init(void)
{
diva_didd_load_time_init();
@@ -107,7 +107,7 @@ int DIVA_INIT_FUNCTION diddfunc_init(void)
/*
* finit
*/
-void DIVA_EXIT_FUNCTION diddfunc_finit(void)
+void __exit diddfunc_finit(void)
{
DbgDeregister();
disconnect_didd();
diff --git a/drivers/isdn/hardware/eicon/diva_didd.c b/drivers/isdn/hardware/eicon/diva_didd.c
index d1d3de0..fab6ccf 100644
--- a/drivers/isdn/hardware/eicon/diva_didd.c
+++ b/drivers/isdn/hardware/eicon/diva_didd.c
@@ -91,7 +91,7 @@ static const struct file_operations divadidd_proc_fops = {
.release = single_release,
};
-static int DIVA_INIT_FUNCTION create_proc(void)
+static int __init create_proc(void)
{
proc_net_eicon = proc_mkdir("eicon", init_net.proc_net);
@@ -109,7 +109,7 @@ static void remove_proc(void)
remove_proc_entry("eicon", init_net.proc_net);
}
-static int DIVA_INIT_FUNCTION divadidd_init(void)
+static int __init divadidd_init(void)
{
char tmprev[32];
int ret = 0;
@@ -141,7 +141,7 @@ out:
return (ret);
}
-static void DIVA_EXIT_FUNCTION divadidd_exit(void)
+static void __exit divadidd_exit(void)
{
diddfunc_finit();
remove_proc();
diff --git a/drivers/isdn/hardware/eicon/divamnt.c b/drivers/isdn/hardware/eicon/divamnt.c
index ffa0c31..48db08d 100644
--- a/drivers/isdn/hardware/eicon/divamnt.c
+++ b/drivers/isdn/hardware/eicon/divamnt.c
@@ -184,7 +184,7 @@ static void divas_maint_unregister_chrdev(void)
unregister_chrdev(major, DEVNAME);
}
-static int DIVA_INIT_FUNCTION divas_maint_register_chrdev(void)
+static int __init divas_maint_register_chrdev(void)
{
if ((major = register_chrdev(0, DEVNAME, &divas_maint_fops)) < 0)
{
@@ -207,7 +207,7 @@ void diva_maint_wakeup_read(void)
/*
* Driver Load
*/
-static int DIVA_INIT_FUNCTION maint_init(void)
+static int __init maint_init(void)
{
char tmprev[50];
int ret = 0;
@@ -245,7 +245,7 @@ out:
/*
** Driver Unload
*/
-static void DIVA_EXIT_FUNCTION maint_exit(void)
+static void __exit maint_exit(void)
{
divas_maint_unregister_chrdev();
mntfunc_finit();
diff --git a/drivers/isdn/hardware/eicon/divasfunc.c b/drivers/isdn/hardware/eicon/divasfunc.c
index 60aaf95..4be5f88 100644
--- a/drivers/isdn/hardware/eicon/divasfunc.c
+++ b/drivers/isdn/hardware/eicon/divasfunc.c
@@ -153,7 +153,7 @@ static void *didd_callback(void *context, DESCRIPTOR *adapter,
/*
* connect to didd
*/
-static int DIVA_INIT_FUNCTION connect_didd(void)
+static int __init connect_didd(void)
{
int x = 0;
int dadapter = 0;
@@ -209,7 +209,7 @@ static void disconnect_didd(void)
/*
* init
*/
-int DIVA_INIT_FUNCTION divasfunc_init(int dbgmask)
+int __init divasfunc_init(int dbgmask)
{
char *version;
diff --git a/drivers/isdn/hardware/eicon/divasi.c b/drivers/isdn/hardware/eicon/divasi.c
index a5c8f90..4103a8c 100644
--- a/drivers/isdn/hardware/eicon/divasi.c
+++ b/drivers/isdn/hardware/eicon/divasi.c
@@ -114,7 +114,7 @@ static const struct file_operations um_idi_proc_fops = {
.release = single_release,
};
-static int DIVA_INIT_FUNCTION create_um_idi_proc(void)
+static int __init create_um_idi_proc(void)
{
um_idi_proc_entry = proc_create(DRIVERLNAME, S_IRUGO, proc_net_eicon,
&um_idi_proc_fops);
@@ -146,7 +146,7 @@ static void divas_idi_unregister_chrdev(void)
unregister_chrdev(major, DEVNAME);
}
-static int DIVA_INIT_FUNCTION divas_idi_register_chrdev(void)
+static int __init divas_idi_register_chrdev(void)
{
if ((major = register_chrdev(0, DEVNAME, &divas_idi_fops)) < 0)
{
@@ -161,7 +161,7 @@ static int DIVA_INIT_FUNCTION divas_idi_register_chrdev(void)
/*
** Driver Load
*/
-static int DIVA_INIT_FUNCTION divasi_init(void)
+static int __init divasi_init(void)
{
char tmprev[50];
int ret = 0;
@@ -202,7 +202,7 @@ out:
/*
** Driver Unload
*/
-static void DIVA_EXIT_FUNCTION divasi_exit(void)
+static void __exit divasi_exit(void)
{
idifunc_finit();
remove_um_idi_proc();
diff --git a/drivers/isdn/hardware/eicon/divasmain.c b/drivers/isdn/hardware/eicon/divasmain.c
index 7eaab06..ca6d276 100644
--- a/drivers/isdn/hardware/eicon/divasmain.c
+++ b/drivers/isdn/hardware/eicon/divasmain.c
@@ -673,7 +673,7 @@ static void divas_unregister_chrdev(void)
unregister_chrdev(major, DEVNAME);
}
-static int DIVA_INIT_FUNCTION divas_register_chrdev(void)
+static int __init divas_register_chrdev(void)
{
if ((major = register_chrdev(0, DEVNAME, &divas_fops)) < 0)
{
@@ -767,7 +767,7 @@ static void __devexit divas_remove_one(struct pci_dev *pdev)
/* --------------------------------------------------------------------------
Driver Load / Startup
-------------------------------------------------------------------------- */
-static int DIVA_INIT_FUNCTION divas_init(void)
+static int __init divas_init(void)
{
char tmprev[50];
int ret = 0;
@@ -831,7 +831,7 @@ out:
/* --------------------------------------------------------------------------
Driver Unload
-------------------------------------------------------------------------- */
-static void DIVA_EXIT_FUNCTION divas_exit(void)
+static void __exit divas_exit(void)
{
pci_unregister_driver(&diva_pci_driver);
remove_divas_proc();
diff --git a/drivers/isdn/hardware/eicon/idifunc.c b/drivers/isdn/hardware/eicon/idifunc.c
index d153e3c..fef6586 100644
--- a/drivers/isdn/hardware/eicon/idifunc.c
+++ b/drivers/isdn/hardware/eicon/idifunc.c
@@ -133,7 +133,7 @@ static void um_remove_card(DESCRIPTOR *d)
/*
* remove all adapter
*/
-static void DIVA_EXIT_FUNCTION remove_all_idi_proc(void)
+static void __exit remove_all_idi_proc(void)
{
udiva_card *card;
diva_os_spin_lock_magic_t old_irql;
@@ -181,7 +181,7 @@ static void *didd_callback(void *context, DESCRIPTOR *adapter,
/*
* connect DIDD
*/
-static int DIVA_INIT_FUNCTION connect_didd(void)
+static int __init connect_didd(void)
{
int x = 0;
int dadapter = 0;
@@ -225,7 +225,7 @@ static int DIVA_INIT_FUNCTION connect_didd(void)
/*
* Disconnect from DIDD
*/
-static void DIVA_EXIT_FUNCTION disconnect_didd(void)
+static void __exit disconnect_didd(void)
{
IDI_SYNC_REQ req;
@@ -240,7 +240,7 @@ static void DIVA_EXIT_FUNCTION disconnect_didd(void)
/*
* init
*/
-int DIVA_INIT_FUNCTION idifunc_init(void)
+int __init idifunc_init(void)
{
diva_os_initialize_spin_lock(&ll_lock, "idifunc");
@@ -260,7 +260,7 @@ int DIVA_INIT_FUNCTION idifunc_init(void)
/*
* finit
*/
-void DIVA_EXIT_FUNCTION idifunc_finit(void)
+void __exit idifunc_finit(void)
{
diva_user_mode_idi_finit();
disconnect_didd();
diff --git a/drivers/isdn/hardware/eicon/mntfunc.c b/drivers/isdn/hardware/eicon/mntfunc.c
index d607260..1cd9aff 100644
--- a/drivers/isdn/hardware/eicon/mntfunc.c
+++ b/drivers/isdn/hardware/eicon/mntfunc.c
@@ -72,7 +72,7 @@ static void *didd_callback(void *context, DESCRIPTOR *adapter,
/*
* connect to didd
*/
-static int DIVA_INIT_FUNCTION connect_didd(void)
+static int __init connect_didd(void)
{
int x = 0;
int dadapter = 0;
@@ -114,7 +114,7 @@ static int DIVA_INIT_FUNCTION connect_didd(void)
/*
* disconnect from didd
*/
-static void DIVA_EXIT_FUNCTION disconnect_didd(void)
+static void __exit disconnect_didd(void)
{
IDI_SYNC_REQ req;
@@ -300,7 +300,7 @@ int maint_read_write(void __user *buf, int count)
/*
* init
*/
-int DIVA_INIT_FUNCTION mntfunc_init(int *buffer_length, void **buffer,
+int __init mntfunc_init(int *buffer_length, void **buffer,
unsigned long diva_dbg_mem)
{
if (*buffer_length < 64) {
@@ -348,7 +348,7 @@ int DIVA_INIT_FUNCTION mntfunc_init(int *buffer_length, void **buffer,
/*
* exit
*/
-void DIVA_EXIT_FUNCTION mntfunc_finit(void)
+void __exit mntfunc_finit(void)
{
void *buffer;
int i = 100;
diff --git a/drivers/isdn/hardware/eicon/platform.h b/drivers/isdn/hardware/eicon/platform.h
index 7331c3b..b2edb75 100644
--- a/drivers/isdn/hardware/eicon/platform.h
+++ b/drivers/isdn/hardware/eicon/platform.h
@@ -38,9 +38,6 @@
#define DIVA_NO_DEBUGLIB
#endif
-#define DIVA_INIT_FUNCTION __init
-#define DIVA_EXIT_FUNCTION __exit
-
#define DIVA_USER_MODE_CARD_CONFIG 1
#define USE_EXTENDED_DEBUGS 1
^ permalink raw reply related
* [PATCH] mISDN: Ignore return value of driver_for_each_device in function hfcpci_softirq.
From: Krzysztof Wilczynski @ 2012-04-24 23:13 UTC (permalink / raw)
To: Karsten Keil
Cc: Greg Kroah-Hartman, Simon Horman, Jiri Kosina, netdev,
linux-kernel
Function hfcpci_softirq uses driver_for_each_device to call _hfcpci_softirq for
each device. Given that _hfcpci_softirq always returns 0, we collect this return
value and simply do nothing with it. This is purely to address the following
warning during compilation time:
drivers/isdn/hardware/mISDN/hfcpci.c: In function ‘hfcpci_softirq’:
drivers/isdn/hardware/mISDN/hfcpci.c:2319: warning: ignoring return value of
‘driver_for_each_device’, declared with attribute warn_unused_result
Similar approach was used in function cx18_alsa_exit from "cx18-alsa-main.c".
Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
---
drivers/isdn/hardware/mISDN/hfcpci.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index e2c83a2..f0d0180 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2316,8 +2316,11 @@ _hfcpci_softirq(struct device *dev, void *arg)
static void
hfcpci_softirq(void *arg)
{
- (void) driver_for_each_device(&hfc_driver.driver, NULL, arg,
- _hfcpci_softirq);
+ int ret;
+
+ /* Collect return value, but do not use it; suppress GCC warning ... */
+ ret = driver_for_each_device(&hfc_driver.driver, NULL, arg,
+ _hfcpci_softirq);
/* if next event would be in the past ... */
if ((s32)(hfc_jiffies + tics - jiffies) <= 0)
--
1.7.2.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox