* Re: [PATCH v2 net-next] net: filter: export pkt_type_offset() helper
From: Alexei Starovoitov @ 2014-09-03 23:14 UTC (permalink / raw)
To: Daniel Borkmann, Eric Dumazet
Cc: Denis Kirjanov, netdev@vger.kernel.org, Markos Chandras,
Martin Schwidefsky
In-Reply-To: <54078FBC.5050402@redhat.com>
On Wed, Sep 3, 2014 at 3:01 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 09/03/2014 11:08 PM, Denis Kirjanov wrote:
>>
>
>> +static int __init init_pkt_type_offset(void)
>> {
>> struct sk_buff skb_probe = { .pkt_type = ~0, };
>> u8 *ct = (u8 *) &skb_probe;
>> unsigned int off;
>>
>> + pkt_type_offset = -1;
>> for (off = 0; off < sizeof(struct sk_buff); off++) {
>> - if (ct[off] == PKT_TYPE_MAX)
>> + if (ct[off] == PKT_TYPE_MAX) {
>> + pkt_type_offset = off;
>> return off;
>> + }
>> }
>
>
> Why not BUG_ON() when pkt_type_offset could not be found?
>
> That way we would know that something is broken and you can
> spare the checks for negative offsets everywhere ...
also such BUG_ON will be right in the face, since the kernel
won't even boot :)
I guess we can only hit it if compiler changes and starts
doing crazing things with bitfields.
Eric, you're the original author of this function, thoughts?
>> case SKF_AD_OFF + SKF_AD_PKTTYPE:
>> + if (pkt_type_offset < 0)
>> + return false;
with BUG_ON we wouldn't need this check.
>> *insn = BPF_LDX_MEM(BPF_B, BPF_REG_A, BPF_REG_CTX,
>> - pkt_type_offset());
>> + pkt_type_offset);
>> if (insn->off < 0)
>> return false;
and this one too.
and other checks in arch/*/jit
^ permalink raw reply
* Re: [patch net-next 07/13] dsa: implement ndo_swdev_get_id
From: Florian Fainelli @ 2014-09-03 23:20 UTC (permalink / raw)
To: Jiri Pirko, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: ryazanov.s.a-Re5JQEeQqe8AvxtiuMwx3w,
jasowang-H+wXaHxf7aLQT0dZR+AlfA,
john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w,
Neil.Jerram-QnUH15yq9NYqDJ6do+/SaQ,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, andy-QlMahl40kYEqcZcGjlUOXw,
dev-yBygre7rU0TnMu66kgdUjQ, nbd-p3rKhJxN3npAfugRpC6u6w,
ronye-VPRAkNaXOzVWk0Htik3J/w,
jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, ben-/+tVBieCtBitmTQ+vhA3Yw,
buytenh-OLH4Qvv75CYX/NnBR394Jw,
roopa-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR,
jhs-jkUAjuhPggJWk0Htik3J/w, aviadr-VPRAkNaXOzVWk0Htik3J/w,
nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w,
vyasevic-H+wXaHxf7aLQT0dZR+AlfA, nhorman-2XuSBdqkA4R54TAoqtyWWQ,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
dborkman-H+wXaHxf7aLQT0dZR+AlfA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <1409736300-12303-8-git-send-email-jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
On 09/03/2014 02:24 AM, Jiri Pirko wrote:
> Signed-off-by: Jiri Pirko <jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
> ---
> include/linux/netdevice.h | 3 ++-
> include/net/dsa.h | 1 +
> net/dsa/Kconfig | 2 +-
> net/dsa/dsa.c | 3 +++
> net/dsa/slave.c | 10 ++++++++++
> 5 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 6a009d1..7ee070f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -41,7 +41,6 @@
>
> #include <linux/ethtool.h>
> #include <net/net_namespace.h>
> -#include <net/dsa.h>
> #ifdef CONFIG_DCB
> #include <net/dcbnl.h>
> #endif
> @@ -1259,6 +1258,8 @@ enum netdev_priv_flags {
> #define IFF_LIVE_ADDR_CHANGE IFF_LIVE_ADDR_CHANGE
> #define IFF_MACVLAN IFF_MACVLAN
>
> +#include <net/dsa.h>
> +
> /**
> * struct net_device - The DEVICE structure.
> * Actually, this whole structure is a big mistake. It mixes I/O
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 9771292..d60cd42 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -140,6 +140,7 @@ struct dsa_switch {
> u32 phys_mii_mask;
> struct mii_bus *slave_mii_bus;
> struct net_device *ports[DSA_MAX_PORTS];
> + struct netdev_phys_item_id psid;
> };
>
> static inline bool dsa_is_cpu_port(struct dsa_switch *ds, int p)
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index a585fd6..4e144a2 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -1,6 +1,6 @@
> config HAVE_NET_DSA
> def_bool y
> - depends on NETDEVICES && !S390
> + depends on NETDEVICES && NET_SWITCHDEV && !S390
It does not look like this is necessary, we are only using definitions
from net/dsa.h and include/linux/netdevice.h, and if it was, a 'select'
would be more appropriate here I think.
TBH, I think we should rather drop this patch for now, I do not see any
benefit in providing a random id over no-id at all.
>
> # Drivers must select NET_DSA and the appropriate tagging format
>
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 61f145c..374912d 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -202,6 +202,9 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
> ds->ports[i] = slave_dev;
> }
>
> + ds->psid.id_len = MAX_PHYS_ITEM_ID_LEN;
> + get_random_bytes(ds->psid.id, ds->psid.id_len);
> +
> return ds;
>
> out_free:
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 7333a4a..d79a6c7 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -192,6 +192,15 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
> return NETDEV_TX_OK;
> }
>
> +static int dsa_slave_swdev_get_id(struct net_device *dev,
> + struct netdev_phys_item_id *psid)
> +{
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + struct dsa_switch *ds = p->parent;
> +
> + memcpy(psid, &ds->psid, sizeof(*psid));
> + return 0;
> +}
>
> /* ethtool operations *******************************************************/
> static int
> @@ -323,6 +332,7 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
> .ndo_set_rx_mode = dsa_slave_set_rx_mode,
> .ndo_set_mac_address = dsa_slave_set_mac_address,
> .ndo_do_ioctl = dsa_slave_ioctl,
> + .ndo_swdev_get_id = dsa_slave_swdev_get_id,
> };
>
> static const struct dsa_device_ops notag_netdev_ops = {
>
^ permalink raw reply
* Re: [PATCH net 1/2] ipv6: fix a refcnt leak with peer addr
From: Hannes Frederic Sowa @ 2014-09-03 23:43 UTC (permalink / raw)
To: Nicolas Dichtel, yoshfuji, davem; +Cc: netdev
In-Reply-To: <1409781562-4191-1-git-send-email-nicolas.dichtel@6wind.com>
On Wed, Sep 3, 2014, at 23:59, Nicolas Dichtel wrote:
> There is no reason to take a refcnt before deleting the peer address
> route.
> It's done some lines below for the local prefix route because
> inet6_ifa_finish_destroy() will release it at the end.
> For the peer address route, we want to free it right now.
>
> This bug has been introduced by commit
> caeaba79009c ("ipv6: add support of peer address").
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
^ permalink raw reply
* [PATCH net-next v2 0/2] Combine standard and phy timestamping logic
From: Alexander Duyck @ 2014-09-03 23:38 UTC (permalink / raw)
To: netdev; +Cc: richardcochran, davem, willemb
This change makes it so that the core path for the phy timestamping logic
is shared between skb_tx_tstamp and skb_complete_tx_timestamp. In addition
it provides a means of using the same skb clone type path in non phy
timestamping drivers.
The main motivation for this is to enable non-phy drivers to be able to
manipulate tx timestamp skbs for such things as putting them in lists or
setting aside buffer in the context block.
---
v2: Incorporated suggested changes from Willem de Bruijn and Eric Dumazet
dropped uneeded comment
restored order of hwtstamp vs swtstamp
added destructor for skb
Dropped usage of skb_complete_tx_timestamp as a kfree_skb w/ destructor
Alexander Duyck (2):
net-timestamp: Merge shared code between phy and regular timestamping
net-timestamp: Make the clone operation stand-alone from phy timestamping
drivers/net/phy/dp83640.c | 6 ++--
include/linux/skbuff.h | 2 +
net/core/skbuff.c | 77 +++++++++++++++++++++++++++++++++------------
net/core/timestamping.c | 43 ++-----------------------
4 files changed, 64 insertions(+), 64 deletions(-)
--
^ permalink raw reply
* [PATCH net-next v2 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping
From: Alexander Duyck @ 2014-09-03 23:42 UTC (permalink / raw)
To: netdev; +Cc: richardcochran, davem, willemb
In-Reply-To: <20140903233447.7517.52329.stgit@ahduyck-bv4.jf.intel.com>
The phy timestamping takes a different path than the regular timestamping
does in that it will create a clone first so that the packets needing to be
timestamped can be placed in a queue, or the context block could be used.
In order to support these use cases I am pulling the core of the code out
so it can be used in other drivers beyond just phy devices.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
v2: Renamed function to skb_clone_sk.
Added destructor to call sock_put instead of doing it ourselves.
Dropped freeing functionality from skb_complete_tx_timestamp.
Added additional documentation to the code.
drivers/net/phy/dp83640.c | 6 +++---
include/linux/skbuff.h | 2 ++
net/core/skbuff.c | 40 ++++++++++++++++++++++++++++------------
net/core/timestamping.c | 14 +++-----------
4 files changed, 36 insertions(+), 26 deletions(-)
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index d5991ac..87648b3 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1148,7 +1148,7 @@ static void dp83640_remove(struct phy_device *phydev)
kfree_skb(skb);
while ((skb = skb_dequeue(&dp83640->tx_queue)) != NULL)
- skb_complete_tx_timestamp(skb, NULL);
+ kfree_skb(skb);
clock = dp83640_clock_get(dp83640->clock);
@@ -1405,7 +1405,7 @@ static void dp83640_txtstamp(struct phy_device *phydev,
case HWTSTAMP_TX_ONESTEP_SYNC:
if (is_sync(skb, type)) {
- skb_complete_tx_timestamp(skb, NULL);
+ kfree_skb(skb);
return;
}
/* fall through */
@@ -1416,7 +1416,7 @@ static void dp83640_txtstamp(struct phy_device *phydev,
case HWTSTAMP_TX_OFF:
default:
- skb_complete_tx_timestamp(skb, NULL);
+ kfree_skb(skb);
break;
}
}
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 02529fc..1cf0cfa 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2690,6 +2690,8 @@ static inline ktime_t net_invalid_timestamp(void)
return ktime_set(0, 0);
}
+struct sk_buff *skb_clone_sk(struct sk_buff *skb);
+
#ifdef CONFIG_NETWORK_PHY_TIMESTAMPING
void skb_clone_tx_timestamp(struct sk_buff *skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 697e696..92c7288 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3511,6 +3511,32 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
}
EXPORT_SYMBOL(sock_dequeue_err_skb);
+static void skb_clone_sk_destructor(struct sk_buff *skb)
+{
+ sock_put(skb->sk);
+}
+
+struct sk_buff *skb_clone_sk(struct sk_buff *skb)
+{
+ struct sock *sk = skb->sk;
+ struct sk_buff *clone;
+
+ if (!sk || !atomic_inc_not_zero(&sk->sk_refcnt))
+ return NULL;
+
+ clone = skb_clone(skb, GFP_ATOMIC);
+ if (!clone) {
+ sock_put(sk);
+ return NULL;
+ }
+
+ clone->sk = sk;
+ clone->destructor = skb_clone_sk_destructor;
+
+ return clone;
+}
+EXPORT_SYMBOL(skb_clone_sk);
+
static void __skb_complete_tx_timestamp(struct sk_buff *skb,
struct sock *sk,
int tstype)
@@ -3538,18 +3564,8 @@ static void __skb_complete_tx_timestamp(struct sk_buff *skb,
void skb_complete_tx_timestamp(struct sk_buff *skb,
struct skb_shared_hwtstamps *hwtstamps)
{
- struct sock *sk = skb->sk;
-
- skb->sk = NULL;
-
- if (hwtstamps) {
- *skb_hwtstamps(skb) = *hwtstamps;
- __skb_complete_tx_timestamp(skb, sk, SCM_TSTAMP_SND);
- } else {
- kfree_skb(skb);
- }
-
- sock_put(sk);
+ *skb_hwtstamps(skb) = *hwtstamps;
+ __skb_complete_tx_timestamp(skb, skb->sk, SCM_TSTAMP_SND);
}
EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index f48a59f..43d3dd6 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -36,10 +36,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
{
struct phy_device *phydev;
struct sk_buff *clone;
- struct sock *sk = skb->sk;
unsigned int type;
- if (!sk)
+ if (!skb->sk)
return;
type = classify(skb);
@@ -48,16 +47,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
phydev = skb->dev->phydev;
if (likely(phydev->drv->txtstamp)) {
- if (!atomic_inc_not_zero(&sk->sk_refcnt))
+ clone = skb_clone_sk(skb);
+ if (!clone)
return;
-
- clone = skb_clone(skb, GFP_ATOMIC);
- if (!clone) {
- sock_put(sk);
- return;
- }
-
- clone->sk = sk;
phydev->drv->txtstamp(phydev, clone, type);
}
}
^ permalink raw reply related
* Re: [PATCH net 2/2] ipv6: use addrconf_get_prefix_route() to remove peer addr
From: Hannes Frederic Sowa @ 2014-09-03 23:51 UTC (permalink / raw)
To: Nicolas Dichtel, yoshfuji, davem; +Cc: netdev
In-Reply-To: <1409781562-4191-2-git-send-email-nicolas.dichtel@6wind.com>
On Wed, Sep 3, 2014, at 23:59, Nicolas Dichtel wrote:
> addrconf_get_prefix_route() ensures to get the right route in the right
> table.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Also good!
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Thanks,
Hannes
^ permalink raw reply
* [PATCH net-next v2 1/2] net-timestamp: Merge shared code between phy and regular timestamping
From: Alexander Duyck @ 2014-09-03 23:39 UTC (permalink / raw)
To: netdev; +Cc: richardcochran, davem, willemb
In-Reply-To: <20140903233447.7517.52329.stgit@ahduyck-bv4.jf.intel.com>
This change merges the shared bits that exist between skb_tx_tstamp and
skb_complete_tx_timestamp. By doing this we can avoid the two diverging as
there were already changes pushed into skb_tx_tstamp that hadn't made it
into the other function.
In addition this resolves issues with the fact that
skb_complete_tx_timestamp was included in linux/skbuff.h even though it was
only compiled in if phy timestamping was enabled.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
v2: Dropped comment and restored original order based on comments
from Willem de Bruijn.
net/core/skbuff.c | 65 ++++++++++++++++++++++++++++++-----------------
net/core/timestamping.c | 29 ---------------------
2 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 53ce536..697e696 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3511,33 +3511,13 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
}
EXPORT_SYMBOL(sock_dequeue_err_skb);
-void __skb_tstamp_tx(struct sk_buff *orig_skb,
- struct skb_shared_hwtstamps *hwtstamps,
- struct sock *sk, int tstype)
+static void __skb_complete_tx_timestamp(struct sk_buff *skb,
+ struct sock *sk,
+ int tstype)
{
struct sock_exterr_skb *serr;
- struct sk_buff *skb;
int err;
- if (!sk)
- return;
-
- if (hwtstamps) {
- *skb_hwtstamps(orig_skb) =
- *hwtstamps;
- } else {
- /*
- * no hardware time stamps available,
- * so keep the shared tx_flags and only
- * store software time stamp
- */
- orig_skb->tstamp = ktime_get_real();
- }
-
- skb = skb_clone(orig_skb, GFP_ATOMIC);
- if (!skb)
- return;
-
serr = SKB_EXT_ERR(skb);
memset(serr, 0, sizeof(*serr));
serr->ee.ee_errno = ENOMSG;
@@ -3554,6 +3534,45 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
if (err)
kfree_skb(skb);
}
+
+void skb_complete_tx_timestamp(struct sk_buff *skb,
+ struct skb_shared_hwtstamps *hwtstamps)
+{
+ struct sock *sk = skb->sk;
+
+ skb->sk = NULL;
+
+ if (hwtstamps) {
+ *skb_hwtstamps(skb) = *hwtstamps;
+ __skb_complete_tx_timestamp(skb, sk, SCM_TSTAMP_SND);
+ } else {
+ kfree_skb(skb);
+ }
+
+ sock_put(sk);
+}
+EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
+
+void __skb_tstamp_tx(struct sk_buff *orig_skb,
+ struct skb_shared_hwtstamps *hwtstamps,
+ struct sock *sk, int tstype)
+{
+ struct sk_buff *skb;
+
+ if (!sk)
+ return;
+
+ if (hwtstamps)
+ *skb_hwtstamps(orig_skb) = *hwtstamps;
+ else
+ orig_skb->tstamp = ktime_get_real();
+
+ skb = skb_clone(orig_skb, GFP_ATOMIC);
+ if (!skb)
+ return;
+
+ __skb_complete_tx_timestamp(skb, sk, tstype);
+}
EXPORT_SYMBOL_GPL(__skb_tstamp_tx);
void skb_tstamp_tx(struct sk_buff *orig_skb,
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index a877039..f48a59f 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -63,35 +63,6 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
}
EXPORT_SYMBOL_GPL(skb_clone_tx_timestamp);
-void skb_complete_tx_timestamp(struct sk_buff *skb,
- struct skb_shared_hwtstamps *hwtstamps)
-{
- struct sock *sk = skb->sk;
- struct sock_exterr_skb *serr;
- int err;
-
- if (!hwtstamps) {
- sock_put(sk);
- kfree_skb(skb);
- return;
- }
-
- *skb_hwtstamps(skb) = *hwtstamps;
-
- serr = SKB_EXT_ERR(skb);
- memset(serr, 0, sizeof(*serr));
- serr->ee.ee_errno = ENOMSG;
- serr->ee.ee_origin = SO_EE_ORIGIN_TIMESTAMPING;
- skb->sk = NULL;
-
- err = sock_queue_err_skb(sk, skb);
-
- sock_put(sk);
- if (err)
- kfree_skb(skb);
-}
-EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
-
bool skb_defer_rx_timestamp(struct sk_buff *skb)
{
struct phy_device *phydev;
^ permalink raw reply related
* Re: [PATCH net-next] sock: consistent errqueue errors and signals
From: Hannes Frederic Sowa @ 2014-09-04 0:17 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David Miller
In-Reply-To: <CA+FuTSfsCiE8HvBHVd1HXG_WXZgZ1D7YjJetmSgsSO46nVtMaA@mail.gmail.com>
Hi,
On Wed, Sep 3, 2014, at 21:40, Willem de Bruijn wrote:
> > I just noticed another problem with this approach why I think we cannot
> > use it.
> >
> > In case we generate an error back to the socket locally because
> > something (e.g. the packet was too big) in output path, we must not
> > update sk->sk_err because we return it immediately to the sender but
> > nonetheless must update the error queue.
> >
> > It seems to me that this patch would report the same error two times to
> > the program then.
>
> For instance in __ip_append_data reaching
>
> ip_local_error(sk, EMSGSIZE, ..);
> return -EMSGSIZE;
>
> With this patch, where sk->sk_err is set, the error will continuously
> be returned on send/recv until the caller reads from the error queue.
> A recvmsg MSG_ERRQUEUE will cause the error to be reset immediately.
sk->sk_err will be reset by sock_error(), called from
__skb_recv_datagram or sock_alloc_send_pskb. So a normal receive or send
(even without MSG_ERRQUEUE) should clean the error code and report it.
No further looks into the error queue should happen currently (I am
currently talking about the patch in this thread).
I was concerned that we report the error twice. Once in the sendmsg path
and again on recvmsg or sendmsg, which finally clears sk_err.
> If socket option IP_RECVERR is set, the process should immediately
> read the error queue when a send call fails with EMSGSIZE or any other
> relevant error. In that case, the error is not reported more than
> once.
I agree but am afraid of applications start to break.
> The alternative patch does not overwrite sk->sk_err, but results in
> this same modified user-visible behavior.
>
> Without the patch, after a send call fails in the above code, the
> process can continue calling send/recv normally while ignoring the
> error, since all "if (sk->sk_err)" checks fall through. If we have to
> treat this as legacy behavior, then neither patch should be applied.
> Then, the semantics are that queued errors are non-blocking. In that
> case, the only patch needed for consistent semantics is to remove the
> sk->sk_err assigment in skb_dequeue_err_skb.
I am afraid that we need to go down this route. :/
I would even let the sk->sk_error_report call be handled by the protocol
error reporting function, as it would need to deal with sk->sk_err, too.
Most applications susceptible to breakage here are ping and
tracepath/traceroute. I don't think they all check MSG_ERRQUEUE always
on every error. But it shouldn't matter that much because they are only
short running apps and error queue will be freed if application exits
before socket accounting will shut down the socket. If we start to
report one instance of an error event multiple times, I think they might
become confused.
> On a related note, when returning an error, returning the errno from
> the item on the queue is a confusing signal. Some error codes are the
> result of protocol processing and have nothing queued (e.g., EINVAL),
> others are due to an queued error (ENOMSG) and some are simply
> ambiguous (EMSGSIZE). It is not reasonable to expect processes to
> figure out the three sets. One solution would be to dedicate a special
> error code to denote "there is a message queued onto sk_error_queue,
> read the actual errno from there". This choice only relevant if we
> decide to return an error, at all, of course.
errno messages are higly sensitive regarding backwards compatibility. I
am totally with you that a special errno marker to indicate that a
packet was enqueued on the error queue would be much better. Currently
applications always have to query error queue on error indication,
otherwise socket accounting might shut down the socket at some point.
Sadly I don't see this change to happen.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH net-next 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-04 0:57 UTC (permalink / raw)
To: Jay Vosburgh
Cc: Veaceslav Falico, Andy Gospodarek, David Miller, netdev,
Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <3024.1409784713@localhost.localdomain>
On Wed, Sep 3, 2014 at 3:51 PM, Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> Mahesh Bandewar <maheshb@google.com> wrote:
>
>>Earlier change to use usable slave array for TLB mode had an additional
>>performance advantage. So extending the same logic to all other modes
>>that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
>>Also consolidating this with the earlier TLB change.
>>
>>The main idea is to build the usable slaves array in the control path
>>and use that array for slave selection during xmit operation.
>>
>>Measured performance in a setup with a bond of 4x1G NICs with 200
>>instances of netperf for the modes involved (3ad, xor, tlb)
>>cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
>>
>>Mode TPS-Before TPS-After
>>
>>802.3ad : 468,694 493,101
>>TLB (lb=0): 392,583 392,965
>>XOR : 475,696 484,517
>
> In general I think this is an improvement over traversing linked
> lists; however, I have a couple of comments.
>
> First, for the 802.3ad mode, is the case within
> bond_3ad_state_machine_handler that runs the ad_agg_selection_logic (if
> the agg_select_timer runs out) handled? This may change the active
> aggregator, and thus the list of slaves that ought to be in the array,
> and looking at the patch I don't see where this would be taken care of.
>
> As a practical matter, the usage of the agg_select_timer is such
> that it's unlikely to cause a change of active aggregator (it will
> usually set an active aggregator when there is none, e.g., during open
> processing), but this case should probably either be handled or
> commented to explain why it is safe.
>
Good point. I'll fix that!
> It may be that the bond_3ad_initialize call to set the
> agg_select_timer to 8 seconds is no longer needed, but for now it's
> there and may change the active aggregator.
>
> One more comment below.
>
>>Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>>---
>> drivers/net/bonding/bond_3ad.c | 77 ++++---------------------------
>> drivers/net/bonding/bond_alb.c | 43 ++---------------
>> drivers/net/bonding/bond_alb.h | 8 ----
>> drivers/net/bonding/bond_main.c | 100 ++++++++++++++++++++++++++++++++++++----
>> drivers/net/bonding/bonding.h | 8 ++++
>> 5 files changed, 113 insertions(+), 123 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>index ee2c73a9de39..d42fd65fdfa9 100644
>>--- a/drivers/net/bonding/bond_3ad.c
>>+++ b/drivers/net/bonding/bond_3ad.c
>>@@ -1717,6 +1717,8 @@ static void ad_enable_collecting_distributing(struct port *port)
>> port->actor_port_number,
>> port->aggregator->aggregator_identifier);
>> __enable_port(port);
>>+ if (bond_update_slave_arr(port->slave->bond, NULL))
>>+ pr_debug("Failed to build slave-array for 3ad mode.\n");
>> }
>> }
>>
>>@@ -1733,6 +1735,8 @@ static void ad_disable_collecting_distributing(struct port *port)
>> port->actor_port_number,
>> port->aggregator->aggregator_identifier);
>> __disable_port(port);
>>+ if (bond_update_slave_arr(port->slave->bond, NULL))
>>+ pr_debug("Failed to build slave-array for 3ad mode.\n");
>> }
>> }
>>
>>@@ -1917,6 +1921,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
>> __update_lacpdu_from_port(port);
>> ad_lacpdu_send(port);
>>
>>+ if (bond_update_slave_arr(bond, slave))
>>+ pr_debug("Failed to build slave-array for 3AD mode.\n");
>>+
>> /* check if this aggregator is occupied */
>> if (aggregator->lag_ports) {
>> /* check if there are other ports related to this aggregator
>>@@ -2311,6 +2318,9 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>> */
>> port->sm_vars |= AD_PORT_BEGIN;
>>
>>+ if (bond_update_slave_arr(slave->bond, NULL))
>>+ pr_debug("Failed to build slave-array for 3ad mode.\n");
>>+
>> __release_state_machine_lock(port);
>> }
>>
>>@@ -2407,73 +2417,6 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>> return ret;
>> }
>>
>>-int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>-{
>>- struct bonding *bond = netdev_priv(dev);
>>- struct slave *slave, *first_ok_slave;
>>- struct aggregator *agg;
>>- struct ad_info ad_info;
>>- struct list_head *iter;
>>- int slaves_in_agg;
>>- int slave_agg_no;
>>- int agg_id;
>>-
>>- if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>- netdev_dbg(dev, "__bond_3ad_get_active_agg_info failed\n");
>>- goto err_free;
>>- }
>>-
>>- slaves_in_agg = ad_info.ports;
>>- agg_id = ad_info.aggregator_id;
>>-
>>- if (slaves_in_agg == 0) {
>>- netdev_dbg(dev, "active aggregator is empty\n");
>>- goto err_free;
>>- }
>>-
>>- slave_agg_no = bond_xmit_hash(bond, skb) % slaves_in_agg;
>>- first_ok_slave = NULL;
>>-
>>- bond_for_each_slave_rcu(bond, slave, iter) {
>>- agg = SLAVE_AD_INFO(slave)->port.aggregator;
>>- if (!agg || agg->aggregator_identifier != agg_id)
>>- continue;
>>-
>>- if (slave_agg_no >= 0) {
>>- if (!first_ok_slave && bond_slave_can_tx(slave))
>>- first_ok_slave = slave;
>>- slave_agg_no--;
>>- continue;
>>- }
>>-
>>- if (bond_slave_can_tx(slave)) {
>>- bond_dev_queue_xmit(bond, skb, slave->dev);
>>- goto out;
>>- }
>>- }
>>-
>>- if (slave_agg_no >= 0) {
>>- netdev_err(dev, "Couldn't find a slave to tx on for aggregator ID %d\n",
>>- agg_id);
>>- goto err_free;
>>- }
>>-
>>- /* we couldn't find any suitable slave after the agg_no, so use the
>>- * first suitable found, if found.
>>- */
>>- if (first_ok_slave)
>>- bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
>>- else
>>- goto err_free;
>>-
>>-out:
>>- return NETDEV_TX_OK;
>>-err_free:
>>- /* no suitable interface, frame not sent */
>>- dev_kfree_skb_any(skb);
>>- goto out;
>>-}
>>-
>> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>> struct slave *slave)
>> {
>>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>index 73c21e233131..b2be7b3216d6 100644
>>--- a/drivers/net/bonding/bond_alb.c
>>+++ b/drivers/net/bonding/bond_alb.c
>>@@ -200,7 +200,6 @@ static int tlb_initialize(struct bonding *bond)
>> static void tlb_deinitialize(struct bonding *bond)
>> {
>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>- struct tlb_up_slave *arr;
>>
>> _lock_tx_hashtbl_bh(bond);
>>
>>@@ -208,10 +207,6 @@ static void tlb_deinitialize(struct bonding *bond)
>> bond_info->tx_hashtbl = NULL;
>>
>> _unlock_tx_hashtbl_bh(bond);
>>-
>>- arr = rtnl_dereference(bond_info->slave_arr);
>>- if (arr)
>>- kfree_rcu(arr, rcu);
>> }
>>
>> static long long compute_gap(struct slave *slave)
>>@@ -1409,39 +1404,9 @@ out:
>> return NETDEV_TX_OK;
>> }
>>
>>-static int bond_tlb_update_slave_arr(struct bonding *bond,
>>- struct slave *skipslave)
>>-{
>>- struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>- struct slave *tx_slave;
>>- struct list_head *iter;
>>- struct tlb_up_slave *new_arr, *old_arr;
>>-
>>- new_arr = kzalloc(offsetof(struct tlb_up_slave, arr[bond->slave_cnt]),
>>- GFP_ATOMIC);
>>- if (!new_arr)
>>- return -ENOMEM;
>>-
>>- bond_for_each_slave(bond, tx_slave, iter) {
>>- if (!bond_slave_can_tx(tx_slave))
>>- continue;
>>- if (skipslave == tx_slave)
>>- continue;
>>- new_arr->arr[new_arr->count++] = tx_slave;
>>- }
>>-
>>- old_arr = rtnl_dereference(bond_info->slave_arr);
>>- rcu_assign_pointer(bond_info->slave_arr, new_arr);
>>- if (old_arr)
>>- kfree_rcu(old_arr, rcu);
>>-
>>- return 0;
>>-}
>>-
>> int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> {
>> struct bonding *bond = netdev_priv(bond_dev);
>>- struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> struct ethhdr *eth_data;
>> struct slave *tx_slave = NULL;
>> u32 hash_index;
>>@@ -1462,9 +1427,9 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> hash_index & 0xFF,
>> skb->len);
>> } else {
>>- struct tlb_up_slave *slaves;
>>+ struct bond_up_slave *slaves;
>>
>>- slaves = rcu_dereference(bond_info->slave_arr);
>>+ slaves = rcu_dereference(bond->slave_arr);
>> if (slaves && slaves->count)
>> tx_slave = slaves->arr[hash_index %
>> slaves->count];
>>@@ -1734,7 +1699,7 @@ void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave)
>> }
>>
>> if (bond_is_nondyn_tlb(bond))
>>- if (bond_tlb_update_slave_arr(bond, slave))
>>+ if (bond_update_slave_arr(bond, slave))
>> pr_err("Failed to build slave-array for TLB mode.\n");
>>
>> }
>>@@ -1762,7 +1727,7 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
>> }
>>
>> if (bond_is_nondyn_tlb(bond)) {
>>- if (bond_tlb_update_slave_arr(bond, NULL))
>>+ if (bond_update_slave_arr(bond, NULL))
>> pr_err("Failed to build slave-array for TLB mode.\n");
>> }
>> }
>>diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
>>index aaeac61d03cf..5fc76c01636c 100644
>>--- a/drivers/net/bonding/bond_alb.h
>>+++ b/drivers/net/bonding/bond_alb.h
>>@@ -139,20 +139,12 @@ struct tlb_slave_info {
>> */
>> };
>>
>>-struct tlb_up_slave {
>>- unsigned int count;
>>- struct rcu_head rcu;
>>- struct slave *arr[0];
>>-};
>>-
>> struct alb_bond_info {
>> struct tlb_client_info *tx_hashtbl; /* Dynamically allocated */
>> spinlock_t tx_hashtbl_lock;
>> u32 unbalanced_load;
>> int tx_rebalance_counter;
>> int lp_counter;
>>- /* -------- non-dynamic tlb mode only ---------*/
>>- struct tlb_up_slave __rcu *slave_arr; /* Up slaves */
>> /* -------- rlb parameters -------- */
>> int rlb_enabled;
>> struct rlb_client_info *rx_hashtbl; /* Receive hash table */
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index f0f5eab0fab1..b0e8e7cfa10f 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1693,6 +1693,9 @@ static int __bond_release_one(struct net_device *bond_dev,
>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> bond_3ad_unbind_slave(slave);
>>
>>+ else if (BOND_MODE(bond) == BOND_MODE_XOR)
>>+ bond_update_slave_arr(bond, slave);
>>+
>> write_unlock_bh(&bond->lock);
>>
>> netdev_info(bond_dev, "Releasing %s interface %s\n",
>>@@ -2009,6 +2012,9 @@ static void bond_miimon_commit(struct bonding *bond)
>> bond_alb_handle_link_change(bond, slave,
>> BOND_LINK_UP);
>>
>>+ if (BOND_MODE(bond) == BOND_MODE_XOR)
>>+ bond_update_slave_arr(bond, NULL);
>>+
>> if (!bond->curr_active_slave ||
>> (slave == bond->primary_slave))
>> goto do_failover;
>>@@ -2037,6 +2043,9 @@ static void bond_miimon_commit(struct bonding *bond)
>> bond_alb_handle_link_change(bond, slave,
>> BOND_LINK_DOWN);
>>
>>+ if (BOND_MODE(bond) == BOND_MODE_XOR)
>>+ bond_update_slave_arr(bond, slave);
>>+
>> if (slave == rcu_access_pointer(bond->curr_active_slave))
>> goto do_failover;
>>
>>@@ -3149,6 +3158,7 @@ static int bond_open(struct net_device *bond_dev)
>> static int bond_close(struct net_device *bond_dev)
>> {
>> struct bonding *bond = netdev_priv(bond_dev);
>>+ struct bond_up_slave *arr;
>>
>> bond_work_cancel_all(bond);
>> bond->send_peer_notif = 0;
>>@@ -3156,6 +3166,10 @@ static int bond_close(struct net_device *bond_dev)
>> bond_alb_deinitialize(bond);
>> bond->recv_probe = NULL;
>>
>>+ arr = rtnl_dereference(bond->slave_arr);
>>+ if (arr)
>>+ kfree_rcu(arr, rcu);
>>+
>> return 0;
>> }
>>
>>@@ -3684,15 +3698,84 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
>> return NETDEV_TX_OK;
>> }
>>
>>-/* In bond_xmit_xor() , we determine the output device by using a pre-
>>- * determined xmit_hash_policy(), If the selected device is not enabled,
>>- * find the next active slave.
>>+/* Build the usable slaves array in control path for modes that use xmit-hash
>>+ * to determine the slave interface -
>>+ * (a) BOND_MODE_8023AD
>>+ * (b) BOND_MODE_XOR
>>+ * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
>> */
>>-static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
>>+int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>> {
>>- struct bonding *bond = netdev_priv(bond_dev);
>>+ struct slave *slave;
>>+ struct list_head *iter;
>>+ struct bond_up_slave *new_arr, *old_arr;
>>+ int slaves_in_agg;
>>+ int agg_id = 0;
>>+ int ret = 0;
>>+
>>+ new_arr = kzalloc(offsetof(struct bond_up_slave, arr[bond->slave_cnt]),
>>+ GFP_ATOMIC);
>>+ if (!new_arr) {
>>+ ret = -ENOMEM;
>>+ goto out;
>>+ }
>
> If this allocation fails, won't the in-place array be left with
> a reference to a slave that has potentially been freed, if, e.g., the
> call came in via the __bond_release_one path?
>
> I haven't tested it, but it seems plausible that we could
> resolve this by writing over the "disappearing" slave's entry in the
> array, e.g., move the last element of the array to the "disappearing"
> element.
>
Allocation failure could be catastrophic. If such a small alloc
request fails, the machine would have to worrying about lot other
things. Having said that, I think we can fix it by overwriting the
disappearing slave with the last entry (as you have suggested).
One other thing I should do is to treat this as an error and use
pr_err() instead of pr_debug() for these alloc failures.
> If this is a problem, the same logic exists in the current
> (tlb-only) code, so a fix for stable may be needed as well.
>
Agreed. I'll cook another patch for the stable branch as well.
> -J
>
>
>>+ if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>+ struct ad_info ad_info;
>>+
>>+ if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>+ pr_debug("__bond_3ad_get_active_agg_info failed\n");
>>+ kfree_rcu(new_arr, rcu);
>>+ ret = -EINVAL;
>>+ goto out;
>>+ }
>>+ slaves_in_agg = ad_info.ports;
>>+ agg_id = ad_info.aggregator_id;
>>+ }
>>+ bond_for_each_slave(bond, slave, iter) {
>>+ if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>+ struct aggregator *agg;
>>+
>>+ agg = SLAVE_AD_INFO(slave)->port.aggregator;
>>+ if (!agg || agg->aggregator_identifier != agg_id)
>>+ continue;
>>+ }
>>+ if (!bond_slave_can_tx(slave))
>>+ continue;
>>+ if (skipslave == slave)
>>+ continue;
>>+ new_arr->arr[new_arr->count++] = slave;
>>+ }
>>
>>- bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) % bond->slave_cnt);
>>+ old_arr = rcu_dereference_protected(bond->slave_arr,
>>+ lockdep_rtnl_is_held() ||
>>+ lockdep_is_held(&bond->lock) ||
>>+ lockdep_is_held(&bond->curr_slave_lock));
>>+ rcu_assign_pointer(bond->slave_arr, new_arr);
>>+ if (old_arr)
>>+ kfree_rcu(old_arr, rcu);
>>+
>>+out:
>>+ return ret;
>>+}
>>+
>>+/* Use this Xmit function for 3AD as well as XOR modes. The current
>>+ * usable slave array is formed in the control path. The xmit function
>>+ * just calculates hash and sends the packet out.
>>+ */
>>+int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
>>+{
>>+ struct bonding *bond = netdev_priv(dev);
>>+ struct slave *slave;
>>+ struct bond_up_slave *slaves;
>>+
>>+ slaves = rcu_dereference(bond->slave_arr);
>>+ if (slaves && slaves->count) {
>>+ slave = slaves->arr[bond_xmit_hash(bond, skb) % slaves->count];
>>+ bond_dev_queue_xmit(bond, skb, slave->dev);
>>+ } else {
>>+ dev_kfree_skb_any(skb);
>>+ atomic_long_inc(&dev->tx_dropped);
>>+ }
>>
>> return NETDEV_TX_OK;
>> }
>>@@ -3794,12 +3877,11 @@ static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev
>> return bond_xmit_roundrobin(skb, dev);
>> case BOND_MODE_ACTIVEBACKUP:
>> return bond_xmit_activebackup(skb, dev);
>>+ case BOND_MODE_8023AD:
>> case BOND_MODE_XOR:
>>- return bond_xmit_xor(skb, dev);
>>+ return bond_3ad_xor_xmit(skb, dev);
>> case BOND_MODE_BROADCAST:
>> return bond_xmit_broadcast(skb, dev);
>>- case BOND_MODE_8023AD:
>>- return bond_3ad_xmit_xor(skb, dev);
>> case BOND_MODE_ALB:
>> return bond_alb_xmit(skb, dev);
>> case BOND_MODE_TLB:
>>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>index aace510d08d1..4a6195c0de60 100644
>>--- a/drivers/net/bonding/bonding.h
>>+++ b/drivers/net/bonding/bonding.h
>>@@ -177,6 +177,12 @@ struct slave {
>> struct kobject kobj;
>> };
>>
>>+struct bond_up_slave {
>>+ unsigned int count;
>>+ struct rcu_head rcu;
>>+ struct slave *arr[0];
>>+};
>>+
>> /*
>> * Link pseudo-state only used internally by monitors
>> */
>>@@ -196,6 +202,7 @@ struct bonding {
>> struct slave __rcu *curr_active_slave;
>> struct slave __rcu *current_arp_slave;
>> struct slave *primary_slave;
>>+ struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves */
>> bool force_primary;
>> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
>> int (*recv_probe)(const struct sk_buff *, struct bonding *,
>>@@ -527,6 +534,7 @@ const char *bond_slave_link_status(s8 link);
>> struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
>> struct net_device *end_dev,
>> int level);
>>+int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
>>
>> #ifdef CONFIG_PROC_FS
>> void bond_create_proc_entry(struct bonding *bond);
>>--
>>2.1.0.rc2.206.gedb03e5
>>
>
> ---
> -Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* Re: [PATCH v2 net-next] net: filter: export pkt_type_offset() helper
From: Eric Dumazet @ 2014-09-04 1:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Eric Dumazet, Denis Kirjanov,
netdev@vger.kernel.org, Markos Chandras, Martin Schwidefsky
In-Reply-To: <CAADnVQKNHL7iY=VHp4bs5k2hSDf8jy7H-0H0SgYjb81S25XB9Q@mail.gmail.com>
On Wed, 2014-09-03 at 16:14 -0700, Alexei Starovoitov wrote:
> On Wed, Sep 3, 2014 at 3:01 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
> > On 09/03/2014 11:08 PM, Denis Kirjanov wrote:
> >>
> >
> >> +static int __init init_pkt_type_offset(void)
> >> {
> >> struct sk_buff skb_probe = { .pkt_type = ~0, };
> >> u8 *ct = (u8 *) &skb_probe;
> >> unsigned int off;
> >>
> >> + pkt_type_offset = -1;
> >> for (off = 0; off < sizeof(struct sk_buff); off++) {
> >> - if (ct[off] == PKT_TYPE_MAX)
> >> + if (ct[off] == PKT_TYPE_MAX) {
> >> + pkt_type_offset = off;
> >> return off;
> >> + }
> >> }
> >
> >
> > Why not BUG_ON() when pkt_type_offset could not be found?
> >
> > That way we would know that something is broken and you can
> > spare the checks for negative offsets everywhere ...
>
> also such BUG_ON will be right in the face, since the kernel
> won't even boot :)
> I guess we can only hit it if compiler changes and starts
> doing crazing things with bitfields.
> Eric, you're the original author of this function, thoughts?
At the time it was written, BPF JIT would have failed and we would
revert to interpreter, thus BUG() was not needed : I used one
pr_err_once()
Note that pkt_type could be moved easily in sk_buff definition, so a
BUG() would be OK after this patch, since the check is done as a
pure_initcall() at boot time.
^ permalink raw reply
* Re: [PATCH v2 net-next] net: filter: export pkt_type_offset() helper
From: Hannes Frederic Sowa @ 2014-09-04 1:25 UTC (permalink / raw)
To: Eric Dumazet, Alexei Starovoitov
Cc: Daniel Borkmann, Eric Dumazet, Denis Kirjanov, netdev,
Markos Chandras, Martin Schwidefsky
In-Reply-To: <1409792893.26422.60.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, Sep 4, 2014, at 03:08, Eric Dumazet wrote:
> On Wed, 2014-09-03 at 16:14 -0700, Alexei Starovoitov wrote:
> > On Wed, Sep 3, 2014 at 3:01 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
> > > On 09/03/2014 11:08 PM, Denis Kirjanov wrote:
> > >>
> > >
> > >> +static int __init init_pkt_type_offset(void)
> > >> {
> > >> struct sk_buff skb_probe = { .pkt_type = ~0, };
> > >> u8 *ct = (u8 *) &skb_probe;
> > >> unsigned int off;
> > >>
> > >> + pkt_type_offset = -1;
> > >> for (off = 0; off < sizeof(struct sk_buff); off++) {
> > >> - if (ct[off] == PKT_TYPE_MAX)
> > >> + if (ct[off] == PKT_TYPE_MAX) {
> > >> + pkt_type_offset = off;
> > >> return off;
> > >> + }
> > >> }
> > >
> > >
> > > Why not BUG_ON() when pkt_type_offset could not be found?
> > >
> > > That way we would know that something is broken and you can
> > > spare the checks for negative offsets everywhere ...
> >
> > also such BUG_ON will be right in the face, since the kernel
> > won't even boot :)
> > I guess we can only hit it if compiler changes and starts
> > doing crazing things with bitfields.
> > Eric, you're the original author of this function, thoughts?
>
> At the time it was written, BPF JIT would have failed and we would
> revert to interpreter, thus BUG() was not needed : I used one
> pr_err_once()
>
> Note that pkt_type could be moved easily in sk_buff definition, so a
> BUG() would be OK after this patch, since the check is done as a
> pure_initcall() at boot time.
Can't we add an address marker to struct sk_buff?
Several possibilities are available:
ptrdiff_t pkt_type_offset[0] before the pkt_type flags field
If one wants to make it more expressive:
typedef struct {} mark_struct_offset;
and add
mark_struct_offset pkt_type_offset;
at appropriate places
Or maybe an anonymous union?
pkt_type_offset would become a simple offsetof(struct sk_buff,
pkt_type_offset) then and there is no need for BUG_ON then.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH] iproute2: Refactor: using new nl_msg struct for the all Netlink handlers
From: Stephen Hemminger @ 2014-09-04 1:33 UTC (permalink / raw)
To: Vadim Kochan; +Cc: netdev
In-Reply-To: <1409777224-7813-1-git-send-email-vadim4j@gmail.com>
On Wed, 3 Sep 2014 23:47:04 +0300
Vadim Kochan <vadim4j@gmail.com> wrote:
> It will easy to extend with new parameters in future.
>
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
Rather than go to all this effort, lets just convert iproute2 to use
a sane netlink library like libmnl.
^ permalink raw reply
* Re: [patch net-next 01/13] openvswitch: split flow structures into ovs specific and generic ones
From: Jamal Hadi Salim @ 2014-09-04 1:54 UTC (permalink / raw)
To: Pravin Shelar
Cc: ryazanov.s.a, jasowang, john.r.fastabend, neil.jerram,
Eric Dumazet, andy, dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
nbd, f.fainelli, Rony Efraim, jeffrey.t.kirsher, Or Gerlitz,
Ben Hutchings, buytenh, Jiri Pirko, roopa, aviadr,
Nicolas Dichtel, vyasevic, nhorman, netdev, Stephen Hemminger,
Daniel Borkmann, ebiederm
In-Reply-To: <CALnjE+qUqSK7kHSi5BZuA0hzFjMcZ8TCTd9JRG1PPmMfDmAQOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 09/03/14 17:59, Pravin Shelar wrote:
> Both of us are saying same thing.
> What I meant was for OVS use-case, where OVS wants to use offload for
> switching flows, vswitchd userspace process can program HW offload
> using kernel HW offload APIs directly from userspace, rather than
> going through OVS kernel module. If user wants to use some other tool,
> then the tool can use same kernel HW offload APIs.
Ok, sorry, you are right - we are saying the same thing.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 3/3] virtio_ring: unify direct/indirect code paths.
From: Andy Lutomirski @ 2014-09-04 1:59 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, Linux Virtualization, Michael S. Tsirkin
In-Reply-To: <1409718556-3041-4-git-send-email-rusty@rustcorp.com.au>
On Tue, Sep 2, 2014 at 9:29 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> virtqueue_add() populates the virtqueue descriptor table from the sgs
> given. If it uses an indirect descriptor table, then it puts a single
> descriptor in the descriptor table pointing to the kmalloc'ed indirect
> table where the sg is populated.
>
> Previously vring_add_indirect() did the allocation and the simple
> linear layout. We replace that with alloc_indirect() which allocates
> the indirect table then chains it like the normal descriptor table so
> we can reuse the core logic.
>
> + if (vq->indirect && total_sg > 1 && vq->vq.num_free)
> + desc = alloc_indirect(total_sg, gfp);
> + else
> + desc = NULL;
> +
> + if (desc) {
> + /* Use a single buffer which doesn't continue */
> + vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
> + vq->vring.desc[head].addr = virt_to_phys(desc);
> + /* avoid kmemleak false positive (hidden by virt_to_phys) */
> + kmemleak_ignore(desc);
> + vq->vring.desc[head].len = total_sg * sizeof(struct vring_desc);
> +
> + /* Set up rest to use this indirect table. */
> + i = 0;
> + total_sg = 1;
This is a little too magical for me. Would it make sense to add a new
variable for this (total_root_descs or something)?
--Andy
^ permalink raw reply
* Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping
From: Eric Dumazet @ 2014-09-04 2:03 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, richardcochran, davem, willemb
In-Reply-To: <54079E0F.5010604@intel.com>
On Wed, 2014-09-03 at 16:02 -0700, Alexander Duyck wrote:
> On 09/03/2014 03:05 PM, Eric Dumazet wrote:
> > On Wed, 2014-09-03 at 14:07 -0700, Eric Dumazet wrote:
> >
> >>
> >> Normally, if one skb holds a reference to a socket, it should have
> >> skb->destructor set to a cleanup function.
> >>
> >> Otherwise, we rely on callers following a convention, to release sk
> >> reference.
> >>
> >> If you believe its needed, it should be dully documented.
> >
> >
> > BTW, skb_orphan() would BUG() on this case (skb->destrutor == NULL and
> > skb->sk != NULL)
>
> That was why we were setting skb->sk to null before passing it to
> sock_queue_err_skb.
Hmpff... This is scary. skb_clone_tx_timestamp() seems the source of
confusion.
> I think I found the reason why things were done the
> way they were. It looks like skb_orphan is called in
> sock_queue_err_skb.
Thats standard practice before setting skb->destructor
> So any destructor added would be fired there before
> before being replaced.
You can use a pair of sock_hold()/sock_put() to guard against early
dismantle. Look at commit da92b194cc36b5dc1fbd
>
>
> The only part I am not sure about is if that would actually be any sort
> of issue. Do we really need to hold the extra reference to the socket
> all the way through the sock_queue_err_skb call or can we just let it
> get dropped with the call to skb_orphan and let the fact that the
> message is being queued be good enough?
> I'm not sure due to the comment about "before exiting rcu section, make
> sure dst is refcounted". It kind of implies I should be able to hand
> off the reference counts without worrying about the socket being freed
> out from under me.
Thats a different concern : dst in input path are rcu protected.
If we queue one skb with an attached dst, we must take a refcount on the
dst, or risk the dst being freed under us, by the time we dequeue
packet.
That will not prevent sk from being destroyed.
^ permalink raw reply
* Re: [PATCH v2 net-next] net: filter: export pkt_type_offset() helper
From: Eric Dumazet @ 2014-09-04 2:05 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Alexei Starovoitov, Daniel Borkmann, Eric Dumazet, Denis Kirjanov,
netdev, Markos Chandras, Martin Schwidefsky
In-Reply-To: <1409793959.3362714.163402573.6A26EDE1@webmail.messagingengine.com>
On Thu, 2014-09-04 at 03:25 +0200, Hannes Frederic Sowa wrote:
> Can't we add an address marker to struct sk_buff?
>
> Several possibilities are available:
>
> ptrdiff_t pkt_type_offset[0] before the pkt_type flags field
>
> If one wants to make it more expressive:
> typedef struct {} mark_struct_offset;
> and add
> mark_struct_offset pkt_type_offset;
> at appropriate places
>
> Or maybe an anonymous union?
>
> pkt_type_offset would become a simple offsetof(struct sk_buff,
> pkt_type_offset) then and there is no need for BUG_ON then.
You can try, and make sure this works on all gcc compilers, even the one
Andrew Morton uses.
And of course, you need to not introduce holes doing so.
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping
From: Alexander Duyck @ 2014-09-04 2:22 UTC (permalink / raw)
To: Alexander Duyck, netdev; +Cc: richardcochran, davem, willemb
In-Reply-To: <20140903233911.7517.59378.stgit@ahduyck-bv4.jf.intel.com>
On 09/03/2014 04:42 PM, Alexander Duyck wrote:
> The phy timestamping takes a different path than the regular timestamping
> does in that it will create a clone first so that the packets needing to be
> timestamped can be placed in a queue, or the context block could be used.
>
> In order to support these use cases I am pulling the core of the code out
> so it can be used in other drivers beyond just phy devices.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
This patch can be ignored. I need to submit a v3 with some fixes for
socket accounting.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v2 net-next] net: filter: export pkt_type_offset() helper
From: Alexei Starovoitov @ 2014-09-04 2:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: Hannes Frederic Sowa, Daniel Borkmann, Eric Dumazet,
Denis Kirjanov, netdev@vger.kernel.org, Markos Chandras,
Martin Schwidefsky
In-Reply-To: <1409796348.26422.76.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Sep 3, 2014 at 7:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-09-04 at 03:25 +0200, Hannes Frederic Sowa wrote:
>
>> Can't we add an address marker to struct sk_buff?
>>
>> Several possibilities are available:
>>
>> ptrdiff_t pkt_type_offset[0] before the pkt_type flags field
>>
>> If one wants to make it more expressive:
>> typedef struct {} mark_struct_offset;
>> and add
>> mark_struct_offset pkt_type_offset;
>> at appropriate places
>>
>> Or maybe an anonymous union?
>>
>> pkt_type_offset would become a simple offsetof(struct sk_buff,
>> pkt_type_offset) then and there is no need for BUG_ON then.
>
>
> You can try, and make sure this works on all gcc compilers, even the one
> Andrew Morton uses.
>
> And of course, you need to not introduce holes doing so.
good points.
I think Hannes's idea is the best. It will help to get rid of helper altogether.
For my bpf syscall proposal I've used anonymous unions and tested
them with gcc 4.2 - 4.9 on x64/i386/arm32/sparc64 and clang.
All compilers were doing just fine. So it should work.
^ permalink raw reply
* Re: [PATCH] net: treewide: Fix typo found in DocBook/networking.xml
From: David Miller @ 2014-09-04 3:38 UTC (permalink / raw)
To: standby24x7; +Cc: linux-kernel, davem, netdev, rdunlap, linux-doc, trivial
In-Reply-To: <1409716755-1994-1-git-send-email-standby24x7@gmail.com>
From: Masanari Iida <standby24x7@gmail.com>
Date: Wed, 3 Sep 2014 12:59:15 +0900
> * When used in combination with the softirq_exit tracepoint
> - * we can determine the softirq handler runtine.
> + * we can determine the softirq handler runtime.
> */
The author of this comment meant to say "routine" not "runtime"
^ permalink raw reply
* Re: [Patch net-next 04/11] net:fec: add multiqueue support
From: Zhi Li @ 2014-09-04 3:41 UTC (permalink / raw)
To: Florian Fainelli
Cc: Frank Li, Duan Fugang-B38611, David Miller,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Shawn Guo,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <540787B9.8090504-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Wed, Sep 3, 2014 at 4:27 PM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 09/03/2014 01:38 PM, Frank Li wrote:
>> From: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>
>> Add multiqueue support, which is compatible with previous single queue
>> driver for FEC/ENET IPs.
>>
>> By default, the tx/rx queue number is 1, user can config the queue number
>> at DTS file like this:
>> fsl,num_tx_queues=<3>;
>> fsl,num_rx_queues=<3>;
>>
>> Ethernet multiqueue mechanism can improve performance in SMP system.
>> For single hw queue, multiqueue can balance cpu loading.
>> For multi hw queues, multiple cores can process network packets in parallel,
>> and refer the article for the detail advantage for multiqueue:
>> http://vger.kernel.org/~davem/davem_nyc09.pdf
>
> You should probably split that patch in a minimum of two, or more
> different parts.
>
> Two parts:
>
> - add TX multiqueue support
> - add RX multiqueue support
>
I understood your concern.
We also need consider logical integration.
Separate this patch just reduce changed lines in a patch.
Most of this patch change is from fep-> to txq-> or rxq->
best regards
Frank Li
> More parts:
>
> - prepare for TX multiqueue (introduce queue structure, definitions)
> - transition TX path to use queue 0
> - add support for the two other TX queues
> - prepare for RX multiqueue (introduce queue structure, definitions)
> - prepare for multiqueue, hardcode RX path to use queue 0
> - add support for the two other RX queues
>
> The number of changes are extremely hard to review...
> --
> Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next PATCH] qdisc: exit case fixes for skb list handling in qdisc layer
From: David Miller @ 2014-09-04 3:42 UTC (permalink / raw)
To: brouer; +Cc: netdev, fw, hannes, dborkman
In-Reply-To: <20140903101250.7668.91659.stgit@dragon>
From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 03 Sep 2014 12:12:50 +0200
> More minor fixes to merge commit 53fda7f7f9e (Merge branch 'xmit_list')
> that allows us to work with a list of SKBs.
>
> Fixing exit cases in qdisc_reset() and qdisc_destroy(), where a
> leftover requeued SKB (qdisc->gso_skb) can have the potential of
> being a skb list, thus use kfree_skb_list().
>
> This is a followup to commit 10770bc2d1 ("qdisc: adjustments for
> API allowing skb list xmits").
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Applied.
^ permalink raw reply
* Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path
From: David Miller @ 2014-09-04 3:43 UTC (permalink / raw)
To: brouer; +Cc: alexander.duyck, netdev, eric.dumazet
In-Reply-To: <20140903155508.23813.75407.stgit@dragon>
From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 03 Sep 2014 17:56:09 +0200
> In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we
> pull them out of the qdisc") the validation code was moved out of
> dev_hard_start_xmit and into dequeue_skb.
>
> However this overlooked the fact that we do not always enqueue
> the skb onto a qdisc. First situation is if qdisc have flag
> TCQ_F_CAN_BYPASS and qdisc is empty. Second situation is if
> there is no qdisc on the device, which is a common case for
> software devices.
>
> Originally spotted and inital patch by Alexander Duyck.
> As a result Alex was seeing issues trying to connect to a
> vhost_net interface after commit 50cbe9ab5f8d was applied.
>
> Added a call to validate_xmit_skb() in __dev_xmit_skb(), in the
> code path for qdiscs with TCQ_F_CAN_BYPASS flag, and in
> __dev_queue_xmit() when no qdisc.
>
> Also handle the error situation where dev_hard_start_xmit() could
> return a skb list, and does not return dev_xmit_complete(rc) and
> falls through to the kfree_skb(), in that situation it should
> call kfree_skb_list().
>
> Fixes: 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them out of the qdisc")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2] openvswitch: distinguish between the dropped and consumed skb
From: David Miller @ 2014-09-04 3:51 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev, tgraf, pshelar
In-Reply-To: <1409737426-22245-1-git-send-email-roy.qing.li@gmail.com>
From: roy.qing.li@gmail.com
Date: Wed, 3 Sep 2014 17:43:45 +0800
> From: Li RongQing <roy.qing.li@gmail.com>
>
> distinguish between the dropped and consumed skb, not assume the skb
> is consumed always
>
> Cc: Thomas Graf <tgraf@noironetworks.com>
> Cc: Pravin Shelar <pshelar@nicira.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next v5] lib/rhashtable: allow user to set the minimum shifts of shrinking
From: David Miller @ 2014-09-04 3:57 UTC (permalink / raw)
To: ying.xue; +Cc: tgraf, netdev
In-Reply-To: <1409707356-3795-1-git-send-email-ying.xue@windriver.com>
From: Ying Xue <ying.xue@windriver.com>
Date: Wed, 3 Sep 2014 09:22:36 +0800
> Although rhashtable library allows user to specify a quiet big size
> for user's created hash table, the table may be shrunk to a
> very small size - HASH_MIN_SIZE(4) after object is removed from
> the table at the first time. Subsequently, even if the total amount
> of objects saved in the table is quite lower than user's initial
> setting in a long time, the hash table size is still dynamically
> adjusted by rhashtable_shrink() or rhashtable_expand() each time
> object is inserted or removed from the table. However, as
> synchronize_rcu() has to be called when table is shrunk or
> expanded by the two functions, we should permit user to set the
> minimum table size through configuring the minimum number of shifts
> according to user specific requirement, avoiding these expensive
> actions of shrinking or expanding because of calling synchronize_rcu().
>
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH v2] 3c59x: avoid panic in boomerang_start_xmit when finding page address:
From: David Miller @ 2014-09-04 3:58 UTC (permalink / raw)
To: nhorman; +Cc: netdev, klassert
In-Reply-To: <1409739445-28021-1-git-send-email-nhorman@tuxdriver.com>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 3 Sep 2014 06:17:25 -0400
> This bug was reported on a very old kernel (RHEL6, 2.6.32-491.el6):
...
> v2) Converted PCI_DMA_TODEVICE to DMA_TO_DEVICE. Thanks Dave!
Ummm, it's still PCI_DMA_TODEVICE.
> - cpu_to_le32(pci_map_single(
> - VORTEX_PCI(vp),
> - (void *)skb_frag_address(frag),
> - skb_frag_size(frag), PCI_DMA_TODEVICE));
> + cpu_to_le32(skb_frag_dma_map(
> + &VORTEX_PCI(vp)->dev,
> + frag,
> + frag->page_offset, frag->size, PCI_DMA_TODEVICE));
^ 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