* [PATCH 13/12] gigaset: add some more CAPI message handling
From: Tilman Schmidt @ 2009-09-24 22:04 UTC (permalink / raw)
To: Karsten Keil, Karsten Keil
Cc: Hansjoerg Lipp, davem, i4ldeveloper, netdev, linux-kernel
In-Reply-To: <20090924-patch-0.tilman@imap.cc>
Rejecting unsupported CAPI messages with code 0x1102 "illegal command
or subcommand" makes some real-world CAPI apps rather unhappy. This
patch adds code to reply to FACILITY_REQ and RESET_B3_REQ messages
with an appropriate "not supported" message, to accept but ignore
FACILITY_RESP, RESET_B3_RESP and MANUFACTURER messages, and to log
any messages still rejected as illegal.
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
drivers/isdn/gigaset/capi.c | 171 +++++++++++++++++++++++++++++++++++++++---
1 files changed, 158 insertions(+), 13 deletions(-)
diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c
index 6ea2b1d..8afff37 100644
--- a/drivers/isdn/gigaset/capi.c
+++ b/drivers/isdn/gigaset/capi.c
@@ -21,6 +21,7 @@
#define CapiNcpiNotSupportedByProtocol 0x0001
#define CapiFlagsNotSupportedByProtocol 0x0002
#define CapiAlertAlreadySent 0x0003
+#define CapiFacilitySpecificFunctionNotSupported 0x3011
/* missing from capicmd.h */
#define CAPI_CONNECT_IND_BASELEN (CAPI_MSG_BASELEN+4+2+8*1)
@@ -31,9 +32,19 @@
#define CAPI_DATA_B3_CONF_LEN (CAPI_MSG_BASELEN+4+2+2)
#define CAPI_DISCONNECT_IND_LEN (CAPI_MSG_BASELEN+4+2)
#define CAPI_DISCONNECT_B3_IND_BASELEN (CAPI_MSG_BASELEN+4+2+1)
+#define CAPI_FACILITY_CONF_BASELEN (CAPI_MSG_BASELEN+4+2+2+1)
/* most _CONF messages contain only Controller/PLCI/NCCI and Info parameters */
#define CAPI_STDCONF_LEN (CAPI_MSG_BASELEN+4+2)
+#define CAPI_FACILITY_HANDSET 0x0000
+#define CAPI_FACILITY_DTMF 0x0001
+#define CAPI_FACILITY_V42BIS 0x0002
+#define CAPI_FACILITY_SUPPSVC 0x0003
+#define CAPI_FACILITY_WAKEUP 0x0004
+#define CAPI_FACILITY_LI 0x0005
+
+#define CAPI_SUPPSVC_GETSUPPORTED 0x0000
+
/* missing from capiutil.h */
#define CAPIMSG_PLCI_PART(m) CAPIMSG_U8(m, 9)
#define CAPIMSG_NCCI_PART(m) CAPIMSG_U16(m, 10)
@@ -1044,6 +1055,108 @@ static void send_conf(struct gigaset_capi_ctr *iif,
}
/*
+ * process FACILITY_REQ message
+ */
+static void do_facility_req(struct gigaset_capi_ctr *iif,
+ struct gigaset_capi_appl *ap,
+ struct sk_buff *skb)
+{
+ struct cardstate *cs = iif->ctr.driverdata;
+ struct sk_buff *cskb;
+ u8 *pparam;
+ unsigned int msgsize = CAPI_FACILITY_CONF_BASELEN;
+ u16 function, info;
+ static u8 confparam[10]; /* max. 9 octets + length byte */
+
+ /* decode message */
+ capi_message2cmsg(&iif->acmsg, skb->data);
+ dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+
+ /*
+ * Facility Request Parameter is not decoded by capi_message2cmsg()
+ * encoding depends on Facility Selector
+ */
+ switch (iif->acmsg.FacilitySelector) {
+ case CAPI_FACILITY_DTMF: /* ToDo */
+ info = CapiFacilityNotSupported;
+ confparam[0] = 2; /* length */
+ /* DTMF information: Unknown DTMF request */
+ capimsg_setu16(confparam, 1, 2);
+ break;
+
+ case CAPI_FACILITY_V42BIS: /* not supported */
+ info = CapiFacilityNotSupported;
+ confparam[0] = 2; /* length */
+ /* V.42 bis information: not available */
+ capimsg_setu16(confparam, 1, 1);
+ break;
+
+ case CAPI_FACILITY_SUPPSVC:
+ /* decode Function parameter */
+ pparam = iif->acmsg.FacilityRequestParameter;
+ if (pparam == NULL || *pparam < 2) {
+ dev_notice(cs->dev, "%s: %s missing\n", "FACILITY_REQ",
+ "Facility Request Parameter");
+ send_conf(iif, ap, skb, CapiIllMessageParmCoding);
+ return;
+ }
+ function = CAPIMSG_U16(pparam, 1);
+ switch (function) {
+ case CAPI_SUPPSVC_GETSUPPORTED:
+ info = CapiSuccess;
+ /* Supplementary Service specific parameter */
+ confparam[3] = 6; /* length */
+ /* Supplementary services info: Success */
+ capimsg_setu16(confparam, 4, CapiSuccess);
+ /* Supported Services: none */
+ capimsg_setu32(confparam, 6, 0);
+ break;
+ /* ToDo: add supported services */
+ default:
+ info = CapiFacilitySpecificFunctionNotSupported;
+ /* Supplementary Service specific parameter */
+ confparam[3] = 2; /* length */
+ /* Supplementary services info: not supported */
+ capimsg_setu16(confparam, 4,
+ CapiSupplementaryServiceNotSupported);
+ }
+
+ /* Facility confirmation parameter */
+ confparam[0] = confparam[3] + 3; /* total length */
+ /* Function: copy from _REQ message */
+ capimsg_setu16(confparam, 1, function);
+ /* Supplementary Service specific parameter already set above */
+ break;
+
+ case CAPI_FACILITY_WAKEUP: /* ToDo */
+ info = CapiFacilityNotSupported;
+ confparam[0] = 2; /* length */
+ /* Number of accepted awake request parameters: 0 */
+ capimsg_setu16(confparam, 1, 0);
+ break;
+
+ default:
+ info = CapiFacilityNotSupported;
+ confparam[0] = 0; /* empty struct */
+ }
+
+ /* send FACILITY_CONF with given Info and confirmation parameter */
+ capi_cmsg_answer(&iif->acmsg);
+ iif->acmsg.Info = info;
+ iif->acmsg.FacilityConfirmationParameter = confparam;
+ msgsize += confparam[0]; /* length */
+ cskb = alloc_skb(msgsize, GFP_ATOMIC);
+ if (!cskb) {
+ dev_err(cs->dev, "%s: out of memory\n", __func__);
+ return;
+ }
+ capi_cmsg2message(&iif->acmsg, __skb_put(cskb, msgsize));
+ dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+ capi_ctr_handle_message(&iif->ctr, ap->id, cskb);
+}
+
+
+/*
* process LISTEN_REQ message
* just store the masks in the application data structure
*/
@@ -1766,7 +1879,28 @@ static void do_data_b3_req(struct gigaset_capi_ctr *iif,
}
/*
- * CAPI message handler: just reply "not supported in current state"
+ * process RESET_B3_REQ message
+ * just always reply "not supported by current protocol"
+ */
+static void do_reset_b3_req(struct gigaset_capi_ctr *iif,
+ struct gigaset_capi_appl *ap,
+ struct sk_buff *skb)
+{
+ /* decode message */
+ capi_message2cmsg(&iif->acmsg, skb->data);
+ dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+ send_conf(iif, ap, skb,
+ CapiResetProcedureNotSupportedByCurrentProtocol);
+}
+
+/*
+ * dump unsupported/ignored messages at most twice per minute,
+ * some apps send those very frequently
+ */
+static unsigned long ignored_msg_dump_time;
+
+/*
+ * unsupported CAPI message handler
*/
static void do_unsupported(struct gigaset_capi_ctr *iif,
struct gigaset_capi_appl *ap,
@@ -1774,7 +1908,8 @@ static void do_unsupported(struct gigaset_capi_ctr *iif,
{
/* decode message */
capi_message2cmsg(&iif->acmsg, skb->data);
- dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+ if (printk_timed_ratelimit(&ignored_msg_dump_time, 30 * 1000))
+ dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
send_conf(iif, ap, skb, CapiMessageNotSupportedInCurrentState);
}
@@ -1785,7 +1920,11 @@ static void do_nothing(struct gigaset_capi_ctr *iif,
struct gigaset_capi_appl *ap,
struct sk_buff *skb)
{
- dump_rawmsg(DEBUG_CMD, __func__, skb->data);
+ if (printk_timed_ratelimit(&ignored_msg_dump_time, 30 * 1000)) {
+ /* decode message */
+ capi_message2cmsg(&iif->acmsg, skb->data);
+ dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+ }
dev_kfree_skb(skb);
}
@@ -1809,6 +1948,7 @@ static struct {
/* most frequent messages first for faster lookup */
{ CAPI_DATA_B3_REQ, do_data_b3_req },
{ CAPI_DATA_B3_RESP, do_data_b3_resp },
+
{ CAPI_ALERT_REQ, do_alert_req },
{ CAPI_CONNECT_ACTIVE_RESP, do_nothing },
{ CAPI_CONNECT_B3_ACTIVE_RESP, do_nothing },
@@ -1821,23 +1961,25 @@ static struct {
{ CAPI_DISCONNECT_B3_RESP, do_nothing },
{ CAPI_DISCONNECT_REQ, do_disconnect_req },
{ CAPI_DISCONNECT_RESP, do_nothing },
- { CAPI_INFO_REQ, do_unsupported },
+ { CAPI_FACILITY_REQ, do_facility_req },
+ { CAPI_FACILITY_RESP, do_nothing },
+ { CAPI_LISTEN_REQ, do_listen_req },
+ { CAPI_SELECT_B_PROTOCOL_REQ, do_unsupported },
+ { CAPI_RESET_B3_REQ, do_reset_b3_req },
+ { CAPI_RESET_B3_RESP, do_nothing },
+
/*
* ToDo: support overlap sending (requires ev-layer state
* machine extension to generate additional ATD commands)
*/
+ { CAPI_INFO_REQ, do_unsupported },
{ CAPI_INFO_RESP, do_nothing },
- { CAPI_LISTEN_REQ, do_listen_req },
- { CAPI_SELECT_B_PROTOCOL_REQ, do_unsupported },
+
/*
- * ToDo:
- * CAPI_FACILITY_REQ
- * CAPI_FACILITY_RESP
- * CAPI_MANUFACTURER_REQ
- * CAPI_MANUFACTURER_RESP
- * CAPI_RESET_B3_REQ
- * CAPI_RESET_B3_RESP
+ * ToDo: what's the proper response for these?
*/
+ { CAPI_MANUFACTURER_REQ, do_nothing },
+ { CAPI_MANUFACTURER_RESP, do_nothing },
};
/* look up handler */
@@ -1887,6 +2029,9 @@ static u16 gigaset_send_message(struct capi_ctr *ctr, struct sk_buff *skb)
handler = lookup_capi_send_handler(CAPIMSG_CMD(skb->data));
if (!handler) {
/* unknown/unsupported message type */
+ if (printk_ratelimit())
+ dev_notice(cs->dev, "%s: unsupported message %u\n",
+ __func__, CAPIMSG_CMD(skb->data));
return CAPI_ILLCMDORSUBCMDORMSGTOSMALL;
}
--
1.6.2.1.214.ge986c
^ permalink raw reply related
* [PATCH 5/4] Documentation: correction to isdn/INTERFACE.CAPI
From: Tilman Schmidt @ 2009-09-24 22:04 UTC (permalink / raw)
To: Karsten Keil, Karsten Keil; +Cc: davem, i4ldeveloper, netdev, linux-kernel
In-Reply-To: <20090924-patch-0.tilman@imap.cc>
Correct the paragraphs describing the _cstruct and _cmstruct types.
The _cstruct representation is in fact used for some struct
parameters containing struct subparameters, too.
Impact: documentation
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
Documentation/isdn/INTERFACE.CAPI | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/Documentation/isdn/INTERFACE.CAPI b/Documentation/isdn/INTERFACE.CAPI
index e6bb1a7..5fe8de5 100644
--- a/Documentation/isdn/INTERFACE.CAPI
+++ b/Documentation/isdn/INTERFACE.CAPI
@@ -217,18 +217,19 @@ u16 for CAPI parameters of type 'word'
u32 for CAPI parameters of type 'dword'
-_cstruct for CAPI parameters of type 'struct' not containing any
- variably-sized (struct) subparameters (eg. 'Called Party Number')
+_cstruct for CAPI parameters of type 'struct'
The member is a pointer to a buffer containing the parameter in
CAPI encoding (length + content). It may also be NULL, which will
be taken to represent an empty (zero length) parameter.
+ Subparameters are stored in encoded form within the content part.
-_cmstruct for CAPI parameters of type 'struct' containing 'struct'
- subparameters ('Additional Info' and 'B Protocol')
+_cmstruct alternative representation for CAPI parameters of type 'struct'
+ (used only for the 'Additional Info' and 'B Protocol' parameters)
The representation is a single byte containing one of the values:
- CAPI_DEFAULT: the parameter is empty
- CAPI_COMPOSE: the values of the subparameters are stored
- individually in the corresponding _cmsg structure members
+ CAPI_DEFAULT: The parameter is empty/absent.
+ CAPI_COMPOSE: The parameter is present.
+ Subparameter values are stored individually in the corresponding
+ _cmsg structure members.
Functions capi_cmsg2message() and capi_message2cmsg() are provided to convert
messages between their transport encoding described in the CAPI 2.0 standard
--
1.6.2.1.214.ge986c
^ permalink raw reply related
* Gigaset & ISDN followup patches
From: Tilman Schmidt @ 2009-09-24 22:03 UTC (permalink / raw)
To: Karsten Keil, Karsten Keil
Cc: Hansjoerg Lipp, davem, i4ldeveloper, netdev, linux-kernel
Karsten,
I'm sending you one followup patch each to the ISDN and Gigaset patch
series for kernel 2.6.32 I sent on 2009-09-19, fixing issues I found
after that submission. I've tentatively tacked them on to the end of
each series as "patch 5/4" and "13/12" but if you prefer I can respin
patch 1 of the ISDN series and patch 12 of the Gigaset series to
incorporate the changes.
Looking forward to seeing these merged,
Tilman
^ permalink raw reply
* Re: [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: Anton Vorontsov @ 2009-09-24 21:30 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: David Miller, linux-pm, netdev
In-Reply-To: <200909242230.33881.rjw@sisk.pl>
On Thu, Sep 24, 2009 at 10:30:33PM +0200, Rafael J. Wysocki wrote:
> On Thursday 24 September 2009, Anton Vorontsov wrote:
> > Following trace pops up if we try to suspend with 3c59x ethernet NIC
> > brought down:
>
> Patch looks good, but IMO it'd be a little effort to convert the driver to
> dev_pm_ops while you're at it (please see r8169 for a working example).
I'd like to avoid putting irrelevant stuff into bugfixes.
Apart from delights as bisecting and revert-only-offending-piece,
keeping bugfixes small and self-sufficient helps to back-port the
fixes to stable/distro kernels. Think of not so old kernels that
don't have dev_pm_ops.
Converting this driver (and others that I'm interested in) to
dev_pm_ops is on my todo list though.
Thanks,
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH] net: Fix sock_wfree() race
From: Eric Dumazet @ 2009-09-24 20:49 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, albcamus, parag.lkml, linux-kernel, netdev
In-Reply-To: <4ABBD18A.70208@gmail.com>
Jarek Poplawski a écrit :
> Eric Dumazet wrote, On 09/23/2009 03:44 PM:
>
> ...
>> Here is the patch for reference :
>>
>> [PATCH] net: Fix sock_wfree() race
>>
>> Commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
>> (net: No more expensive sock_hold()/sock_put() on each tx)
>> opens a window in sock_wfree() where another cpu
>> might free the socket we are working on.
>>
>> A fix is to call sk->sk_write_space(sk) while still
>> holding a reference on sk.
>>
>>
>> Reported-by: Jike Song <albcamus@gmail.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>> net/core/sock.c | 19 ++++++++++++-------
>> 1 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 30d5446..e1f034e 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -1228,17 +1228,22 @@ void __init sk_init(void)
>> void sock_wfree(struct sk_buff *skb)
>> {
>> struct sock *sk = skb->sk;
>> - int res;
>> + unsigned int len = skb->truesize;
>>
>> - /* In case it might be waiting for more memory. */
>> - res = atomic_sub_return(skb->truesize, &sk->sk_wmem_alloc);
>> - if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE))
>> + if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE)) {
>> + /*
>> + * Keep a reference on sk_wmem_alloc, this will be released
>> + * after sk_write_space() call
>> + */
>> + atomic_sub(len - 1, &sk->sk_wmem_alloc);
>> sk->sk_write_space(sk);
>> + len = 1;
>> + }
>> /*
>> - * if sk_wmem_alloc reached 0, we are last user and should
>> - * free this sock, as sk_free() call could not do it.
>> + * if sk_wmem_alloc reaches 0, we must finish what sk_free()
>> + * could not do because of in-flight packets
>> */
>> - if (res == 0)
>> + if (atomic_sub_return(len, &sk->sk_wmem_alloc) == 0)
>> __sk_free(sk);
>
>
> Probably atomic_sub_and_test() is more popular for this.
Indeed, as x86 can generate faster code (no need of xadd instruction)
Thanks Jarek
[PATCH] net: Fix sock_wfree() race
Commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
(net: No more expensive sock_hold()/sock_put() on each tx)
opens a window in sock_wfree() where another cpu
might free the socket we are working on.
A fix is to call sk->sk_write_space(sk) while still
holding a reference on sk.
Reported-by: Jike Song <albcamus@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/core/sock.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c
index 30d5446..e1f034e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1228,17 +1228,22 @@ void __init sk_init(void)
void sock_wfree(struct sk_buff *skb)
{
struct sock *sk = skb->sk;
- int res;
+ unsigned int len = skb->truesize;
- /* In case it might be waiting for more memory. */
- res = atomic_sub_return(skb->truesize, &sk->sk_wmem_alloc);
- if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE))
+ if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE)) {
+ /*
+ * Keep a reference on sk_wmem_alloc, this will be released
+ * after sk_write_space() call
+ */
+ atomic_sub(len - 1, &sk->sk_wmem_alloc);
sk->sk_write_space(sk);
+ len = 1;
+ }
/*
- * if sk_wmem_alloc reached 0, we are last user and should
- * free this sock, as sk_free() call could not do it.
+ * if sk_wmem_alloc reaches 0, we must finish what sk_free()
+ * could not do because of in-flight packets
*/
- if (res == 0)
+ if (atomic_sub_and_test(len, &sk->sk_wmem_alloc))
__sk_free(sk);
}
EXPORT_SYMBOL(sock_wfree);
^ permalink raw reply related
* Re: [PATCH] inet_peer: Optimize inet_getid()
From: Eric Dumazet @ 2009-09-24 20:44 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <20090924123020.59a2ee6e@s6510>
Stephen Hemminger a écrit :
> On Thu, 24 Sep 2009 21:04:56 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> While investigating for network latencies, I found inet_getid() was a contention point
>> for some workloads.
>>
>> Fix is straightforward, using cmpxchg() instead of
>> a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
>>
>> Another possibility was to use an atomic_t and atomic_add_return() but
>> the size of struct inet_peer object would had doubled on x86_64 because of
>> SLAB_HWCACHE_ALIGN constraint.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> I thought cmpxchg was not available on all architectures.
Here is the second version of the patch, thanks Stephen
[PATCH] inet_peer: Optimize inet_getid()
While investigating for network latencies, I found inet_getid() was a contention point
for some workloads.
If __HAVE_ARCH_CMPXCHG is defined, we can use cmpxchg() instead of
a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
On other arches, we can use an atomic_t instead of a u16,
and atomic_add_return(). This might grow memory usage a bit, unless
someone invents atomic16_t :)
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/inetpeer.h | 43 ++++++++++++++++++++++++++++++++-------
net/ipv4/inetpeer.c | 5 ----
net/ipv4/route.c | 2 -
3 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index 15e1f8f..f1cde51 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -19,7 +19,11 @@ struct inet_peer
struct inet_peer *avl_left, *avl_right;
__be32 v4daddr; /* peer's address */
__u16 avl_height;
+#if defined(__HAVE_ARCH_CMPXCHG)
__u16 ip_id_count; /* IP ID for the next packet */
+#else
+ atomic_t ip_id_count;
+#endif
struct list_head unused;
__u32 dtime; /* the time of last use of not
* referenced entries */
@@ -37,17 +41,42 @@ struct inet_peer *inet_getpeer(__be32 daddr, int create);
/* can be called from BH context or outside */
extern void inet_putpeer(struct inet_peer *p);
-extern spinlock_t inet_peer_idlock;
/* can be called with or without local BH being disabled */
static inline __u16 inet_getid(struct inet_peer *p, int more)
{
- __u16 id;
+ __u16 old;
- spin_lock_bh(&inet_peer_idlock);
- id = p->ip_id_count;
- p->ip_id_count += 1 + more;
- spin_unlock_bh(&inet_peer_idlock);
- return id;
+ more++;
+#if defined(__HAVE_ARCH_CMPXCHG)
+ while (1) {
+ old = p->ip_id_count;
+ if (cmpxchg(&p->ip_id_count, old, old + more) == old)
+ break;
+ }
+#else
+ old = atomic_add_return(more, &p->ip_id_count) - more;
+#endif
+ return old;
}
+static inline void inet_id_set(struct inet_peer *p, int val)
+{
+#if defined(__HAVE_ARCH_CMPXCHG)
+ p->ip_id_count = val;
+#else
+ atomic_set(&p->ip_id_count, val);
+#endif
+}
+
+static inline __u16 inet_id_read(const struct inet_peer *p)
+{
+#if defined(__HAVE_ARCH_CMPXCHG)
+ return p->ip_id_count;
+#else
+ return atomic_read(&p->ip_id_count);
+#endif
+}
+
+
+
#endif /* _NET_INETPEER_H */
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index b1fbe18..3641831 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -67,9 +67,6 @@
* ip_id_count: idlock
*/
-/* Exported for inet_getid inline function. */
-DEFINE_SPINLOCK(inet_peer_idlock);
-
static struct kmem_cache *peer_cachep __read_mostly;
#define node_height(x) x->avl_height
@@ -390,7 +387,7 @@ struct inet_peer *inet_getpeer(__be32 daddr, int create)
n->v4daddr = daddr;
atomic_set(&n->refcnt, 1);
atomic_set(&n->rid, 0);
- n->ip_id_count = secure_ip_id(daddr);
+ inet_id_set(n, secure_ip_id(daddr));
n->tcp_ts_stamp = 0;
write_lock_bh(&peer_pool_lock);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index df93473..a090fcf 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2855,7 +2855,7 @@ static int rt_fill_info(struct net *net,
error = rt->u.dst.error;
expires = rt->u.dst.expires ? rt->u.dst.expires - jiffies : 0;
if (rt->peer) {
- id = rt->peer->ip_id_count;
+ id = inet_id_read(rt->peer);
if (rt->peer->tcp_ts_stamp) {
ts = rt->peer->tcp_ts;
tsage = get_seconds() - rt->peer->tcp_ts_stamp;
^ permalink raw reply related
* Re: [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: Rafael J. Wysocki @ 2009-09-24 20:30 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: netdev, linux-pm, David Miller
In-Reply-To: <20090924183152.GA30254@oksana.dev.rtsoft.ru>
On Thursday 24 September 2009, Anton Vorontsov wrote:
> Following trace pops up if we try to suspend with 3c59x ethernet NIC
> brought down:
Patch looks good, but IMO it'd be a little effort to convert the driver to
dev_pm_ops while you're at it (please see r8169 for a working example).
Thanks,
Rafael
> root@b1:~# ifconfig eth16 down
> root@b1:~# echo mem > /sys/power/state
> ...
> 3c59x 0000:00:10.0: suspend
> 3c59x 0000:00:10.0: PME# disabled
> Trying to free already-free IRQ 48
> ------------[ cut here ]------------
> Badness at c00554e4 [verbose debug info unavailable]
> NIP: c00554e4 LR: c00554e4 CTR: c019a098
> REGS: c7975c60 TRAP: 0700 Not tainted (2.6.31-rc4)
> MSR: 00021032 <ME,CE,IR,DR> CR: 28242422 XER: 20000000
> TASK = c79cb0c0[1746] 'bash' THREAD: c7974000
> ...
> NIP [c00554e4] __free_irq+0x108/0x1b0
> LR [c00554e4] __free_irq+0x108/0x1b0
> Call Trace:
> [c7975d10] [c00554e4] __free_irq+0x108/0x1b0 (unreliable)
> [c7975d30] [c005559c] free_irq+0x10/0x24
> [c7975d40] [c01e21ec] vortex_suspend+0x70/0xc4
> [c7975d60] [c017e584] pci_legacy_suspend+0x58/0x100
>
> This is because the driver manages interrupts without checking for
> netif_running().
>
> Though, there are few other issues with suspend/resume in this driver.
> The intention of calling free_irq() in suspend() was to avoid any
> possible spurious interrupts (see commit 5b039e681b8c5f30aac9cc04385
> "3c59x PM fixes"). But,
>
> - On resume, the driver was requesting IRQ just after pci_set_master(),
> but before vortex_up() (which actually resets 3c59x chips).
>
> - Issuing free_irq() on a shared IRQ doesn't guarantee that a buggy
> HW won't trigger spurious interrupts in another driver that
> requested the same interrupt. So, if we want to protect from
> unexpected interrupts, then on suspend we should issue disable_irq(),
> not free_irq().
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> drivers/net/3c59x.c | 12 +++---------
> 1 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> index c34aee9..7cdd4b0 100644
> --- a/drivers/net/3c59x.c
> +++ b/drivers/net/3c59x.c
> @@ -807,10 +807,10 @@ static int vortex_suspend(struct pci_dev *pdev, pm_message_t state)
> if (netif_running(dev)) {
> netif_device_detach(dev);
> vortex_down(dev, 1);
> + disable_irq(dev->irq);
> }
> pci_save_state(pdev);
> pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> - free_irq(dev->irq, dev);
> pci_disable_device(pdev);
> pci_set_power_state(pdev, pci_choose_state(pdev, state));
> }
> @@ -833,18 +833,12 @@ static int vortex_resume(struct pci_dev *pdev)
> return err;
> }
> pci_set_master(pdev);
> - if (request_irq(dev->irq, vp->full_bus_master_rx ?
> - &boomerang_interrupt : &vortex_interrupt, IRQF_SHARED, dev->name, dev)) {
> - pr_warning("%s: Could not reserve IRQ %d\n", dev->name, dev->irq);
> - pci_disable_device(pdev);
> - return -EBUSY;
> - }
> if (netif_running(dev)) {
> err = vortex_up(dev);
> if (err)
> return err;
> - else
> - netif_device_attach(dev);
> + enable_irq(dev->irq);
> + netif_device_attach(dev);
> }
> }
> return 0;
^ permalink raw reply
* Re: [PATCH] net: Fix sock_wfree() race
From: Jarek Poplawski @ 2009-09-24 20:07 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, albcamus, parag.lkml, linux-kernel, netdev
In-Reply-To: <4ABA262F.9040407@gmail.com>
Eric Dumazet wrote, On 09/23/2009 03:44 PM:
...
> Here is the patch for reference :
>
> [PATCH] net: Fix sock_wfree() race
>
> Commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
> (net: No more expensive sock_hold()/sock_put() on each tx)
> opens a window in sock_wfree() where another cpu
> might free the socket we are working on.
>
> A fix is to call sk->sk_write_space(sk) while still
> holding a reference on sk.
>
>
> Reported-by: Jike Song <albcamus@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/core/sock.c | 19 ++++++++++++-------
> 1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 30d5446..e1f034e 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1228,17 +1228,22 @@ void __init sk_init(void)
> void sock_wfree(struct sk_buff *skb)
> {
> struct sock *sk = skb->sk;
> - int res;
> + unsigned int len = skb->truesize;
>
> - /* In case it might be waiting for more memory. */
> - res = atomic_sub_return(skb->truesize, &sk->sk_wmem_alloc);
> - if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE))
> + if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE)) {
> + /*
> + * Keep a reference on sk_wmem_alloc, this will be released
> + * after sk_write_space() call
> + */
> + atomic_sub(len - 1, &sk->sk_wmem_alloc);
> sk->sk_write_space(sk);
> + len = 1;
> + }
> /*
> - * if sk_wmem_alloc reached 0, we are last user and should
> - * free this sock, as sk_free() call could not do it.
> + * if sk_wmem_alloc reaches 0, we must finish what sk_free()
> + * could not do because of in-flight packets
> */
> - if (res == 0)
> + if (atomic_sub_return(len, &sk->sk_wmem_alloc) == 0)
> __sk_free(sk);
Probably atomic_sub_and_test() is more popular for this.
Jarek P.
> }
> EXPORT_SYMBOL(sock_wfree);
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] inet_peer: Optimize inet_getid()
From: Eric Dumazet @ 2009-09-24 19:57 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <20090924123020.59a2ee6e@s6510>
Stephen Hemminger a écrit :
> On Thu, 24 Sep 2009 21:04:56 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> While investigating for network latencies, I found inet_getid() was a contention point
>> for some workloads.
>>
>> Fix is straightforward, using cmpxchg() instead of
>> a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
>>
>> Another possibility was to use an atomic_t and atomic_add_return() but
>> the size of struct inet_peer object would had doubled on x86_64 because of
>> SLAB_HWCACHE_ALIGN constraint.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> I thought cmpxchg was not available on all architectures.
Good point Stephen, I forgot about non x86 arches ;)
I'll send an update with cmpxchg() if available, and atomic_t as fallback.
^ permalink raw reply
* Re: question on raw sockets and source IP address validation
From: Chris Friesen @ 2009-09-24 19:37 UTC (permalink / raw)
To: Neil Horman; +Cc: Linux Network Development list
In-Reply-To: <20090924192651.GC19787@hmsreliant.think-freely.org>
On 09/24/2009 01:26 PM, Neil Horman wrote:
> That said, its not doing source validation, your socket is actually doing a
> route lookup on the flow from your specified source address to your destination
> address. So you should be able to fool the socket into doing the lookup by
> adding a route to your routing table from your source address to your
> destination address via the interface that you want to send the frames out of.
Hmm...that's an interesting point. Worth investigating for sure.
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH] inet_peer: Optimize inet_getid()
From: Stephen Hemminger @ 2009-09-24 19:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <4ABBC2D8.2040901@gmail.com>
On Thu, 24 Sep 2009 21:04:56 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> While investigating for network latencies, I found inet_getid() was a contention point
> for some workloads.
>
> Fix is straightforward, using cmpxchg() instead of
> a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
>
> Another possibility was to use an atomic_t and atomic_add_return() but
> the size of struct inet_peer object would had doubled on x86_64 because of
> SLAB_HWCACHE_ALIGN constraint.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
I thought cmpxchg was not available on all architectures.
^ permalink raw reply
* Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
From: Ira W. Snyder @ 2009-09-24 19:27 UTC (permalink / raw)
To: Avi Kivity
Cc: Gregory Haskins, Michael S. Tsirkin, netdev, virtualization, kvm,
linux-kernel, mingo, linux-mm, akpm, hpa, Rusty Russell, s.hetze,
alacrityvm-devel
In-Reply-To: <4ABB1D44.5000007@redhat.com>
On Thu, Sep 24, 2009 at 10:18:28AM +0300, Avi Kivity wrote:
> On 09/24/2009 12:15 AM, Gregory Haskins wrote:
> >
> >>> There are various aspects about designing high-performance virtual
> >>> devices such as providing the shortest paths possible between the
> >>> physical resources and the consumers. Conversely, we also need to
> >>> ensure that we meet proper isolation/protection guarantees at the same
> >>> time. What this means is there are various aspects to any
> >>> high-performance PV design that require to be placed in-kernel to
> >>> maximize the performance yet properly isolate the guest.
> >>>
> >>> For instance, you are required to have your signal-path (interrupts and
> >>> hypercalls), your memory-path (gpa translation), and
> >>> addressing/isolation model in-kernel to maximize performance.
> >>>
> >>>
> >> Exactly. That's what vhost puts into the kernel and nothing more.
> >>
> > Actually, no. Generally, _KVM_ puts those things into the kernel, and
> > vhost consumes them. Without KVM (or something equivalent), vhost is
> > incomplete. One of my goals with vbus is to generalize the "something
> > equivalent" part here.
> >
>
> I don't really see how vhost and vbus are different here. vhost expects
> signalling to happen through a couple of eventfds and requires someone
> to supply them and implement kernel support (if needed). vbus requires
> someone to write a connector to provide the signalling implementation.
> Neither will work out-of-the-box when implementing virtio-net over
> falling dominos, for example.
>
> >>> Vbus accomplishes its in-kernel isolation model by providing a
> >>> "container" concept, where objects are placed into this container by
> >>> userspace. The host kernel enforces isolation/protection by using a
> >>> namespace to identify objects that is only relevant within a specific
> >>> container's context (namely, a "u32 dev-id"). The guest addresses the
> >>> objects by its dev-id, and the kernel ensures that the guest can't
> >>> access objects outside of its dev-id namespace.
> >>>
> >>>
> >> vhost manages to accomplish this without any kernel support.
> >>
> > No, vhost manages to accomplish this because of KVMs kernel support
> > (ioeventfd, etc). Without a KVM-like in-kernel support, vhost is a
> > merely a kind of "tuntap"-like clone signalled by eventfds.
> >
>
> Without a vbus-connector-falling-dominos, vbus-venet can't do anything
> either. Both vhost and vbus need an interface, vhost's is just narrower
> since it doesn't do configuration or enumeration.
>
> > This goes directly to my rebuttal of your claim that vbus places too
> > much in the kernel. I state that, one way or the other, address decode
> > and isolation _must_ be in the kernel for performance. Vbus does this
> > with a devid/container scheme. vhost+virtio-pci+kvm does it with
> > pci+pio+ioeventfd.
> >
>
> vbus doesn't do kvm guest address decoding for the fast path. It's
> still done by ioeventfd.
>
> >> The guest
> >> simply has not access to any vhost resources other than the guest->host
> >> doorbell, which is handed to the guest outside vhost (so it's somebody
> >> else's problem, in userspace).
> >>
> > You mean _controlled_ by userspace, right? Obviously, the other side of
> > the kernel still needs to be programmed (ioeventfd, etc). Otherwise,
> > vhost would be pointless: e.g. just use vanilla tuntap if you don't need
> > fast in-kernel decoding.
> >
>
> Yes (though for something like level-triggered interrupts we're probably
> keeping it in userspace, enjoying the benefits of vhost data path while
> paying more for signalling).
>
> >>> All that is required is a way to transport a message with a "devid"
> >>> attribute as an address (such as DEVCALL(devid)) and the framework
> >>> provides the rest of the decode+execute function.
> >>>
> >>>
> >> vhost avoids that.
> >>
> > No, it doesn't avoid it. It just doesn't specify how its done, and
> > relies on something else to do it on its behalf.
> >
>
> That someone else can be in userspace, apart from the actual fast path.
>
> > Conversely, vbus specifies how its done, but not how to transport the
> > verb "across the wire". That is the role of the vbus-connector abstraction.
> >
>
> So again, vbus does everything in the kernel (since it's so easy and
> cheap) but expects a vbus-connector. vhost does configuration in
> userspace (since it's so clunky and fragile) but expects a couple of
> eventfds.
>
> >>> Contrast this to vhost+virtio-pci (called simply "vhost" from here).
> >>>
> >>>
> >> It's the wrong name. vhost implements only the data path.
> >>
> > Understood, but vhost+virtio-pci is what I am contrasting, and I use
> > "vhost" for short from that point on because I am too lazy to type the
> > whole name over and over ;)
> >
>
> If you #define A A+B+C don't expect intelligent conversation afterwards.
>
> >>> It is not immune to requiring in-kernel addressing support either, but
> >>> rather it just does it differently (and its not as you might expect via
> >>> qemu).
> >>>
> >>> Vhost relies on QEMU to render PCI objects to the guest, which the guest
> >>> assigns resources (such as BARs, interrupts, etc).
> >>>
> >> vhost does not rely on qemu. It relies on its user to handle
> >> configuration. In one important case it's qemu+pci. It could just as
> >> well be the lguest launcher.
> >>
> > I meant vhost=vhost+virtio-pci here. Sorry for the confusion.
> >
> > The point I am making specifically is that vhost in general relies on
> > other in-kernel components to function. I.e. It cannot function without
> > having something like the PCI model to build an IO namespace. That
> > namespace (in this case, pio addresses+data tuples) are used for the
> > in-kernel addressing function under KVM + virtio-pci.
> >
> > The case of the lguest launcher is a good one to highlight. Yes, you
> > can presumably also use lguest with vhost, if the requisite facilities
> > are exposed to lguest-bus, and some eventfd based thing like ioeventfd
> > is written for the host (if it doesnt exist already).
> >
> > And when the next virt design "foo" comes out, it can make a "foo-bus"
> > model, and implement foo-eventfd on the backend, etc, etc.
> >
>
> It's exactly the same with vbus needing additional connectors for
> additional transports.
>
> > Ira can make ira-bus, and ira-eventfd, etc, etc.
> >
> > Each iteration will invariably introduce duplicated parts of the stack.
> >
>
> Invariably? Use libraries (virtio-shmem.ko, libvhost.so).
>
Referencing libraries that don't yet exist doesn't seem like a good
argument against vbus from my point of view. I'm not speficially
advocating for vbus; I'm just letting you know how it looks to another
developer in the trenches.
If you'd like to see the amount of duplication present, look at the code
I'm currently working on. It mostly works at this point, though I
haven't finished my userspace, nor figured out how to actually transfer
data.
The current question I have (just to let you know where I am in
development) is:
I have the physical address of the remote data, but how do I get it into
a userspace buffer, so I can pass it to tun?
http://www.mmarray.org/~iws/virtio-phys/
Ira
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: question on raw sockets and source IP address validation
From: Neil Horman @ 2009-09-24 19:26 UTC (permalink / raw)
To: Chris Friesen; +Cc: Linux Network Development list
In-Reply-To: <4ABBB223.8090700@nortel.com>
On Thu, Sep 24, 2009 at 11:53:39AM -0600, Chris Friesen wrote:
> Hi all,
>
> Normally when sending a packet on a SOCK_RAW socket the source IP
> address is validated against the addresses configured on the host. If
> the address isn't configured, the packet isn't sent.
>
> This can be avoided by setting IP_HDRINCL, but then the app needs to
> handle all the fragmentation itself.
>
> Is there any way to bypass the source address validation without IP_HDRINCL?
>
Nope, not with socket(AF_INET, SOCK_RAW, ...). its an IPv4 socket, so you get
ipv4 routing. If you don't want the ipv4 behavior, you can always use
AF_PACKET, to send raw frames direct to network interfaces. Of course, thats
going to imply that you do all your ip level fragmentation yourself as well.
That said, its not doing source validation, your socket is actually doing a
route lookup on the flow from your specified source address to your destination
address. So you should be able to fool the socket into doing the lookup by
adding a route to your routing table from your source address to your
destination address via the interface that you want to send the frames out of.
Regards
Neil
> Thanks,
>
> Chris
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: ixgbe patch to provide NIC's tx/rx counters via ethtool
From: Jeff Kirsher @ 2009-09-24 19:10 UTC (permalink / raw)
To: Peter P Waskiewicz Jr; +Cc: Ben Greear, NetDev
In-Reply-To: <1253816908.3153.4.camel@localhost.localdomain>
On Thu, Sep 24, 2009 at 11:28, Peter P Waskiewicz Jr
<peter.p.waskiewicz.jr@intel.com> wrote:
> On Wed, 2009-09-23 at 15:36 -0700, Ben Greear wrote:
>> When LRO is enabled, the received packet and byte counters represent the
>> LRO'd packets, not the packets/bytes on the wire. The Intel 82599 NIC has
>> registers that keep count of the physical packets. Add these counters to
>> the ethtool stats. The byte counters are 36-bit, but the high 4 bits were
>> being ignored in the 2.6.31 ixgbe driver: Read those as well to allow
>> longer time between polling the stats to detect wraps.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>
>>
>> Please do not apply this until the ixgbe authors ACK it. There may
>> have been reasons for not reading the high 4 bits, or they may dislike
>> this approach entirely.
>
> Aside from the trivial line-wrap on the comments, I'm fine with this
> patch. There is no issue I could find with the hardware that would
> limit you from reading the high 4 bits. And since we're reading it
> already to clear the register, we might as well use the value we get
> from it.
>
> Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>
I have added this patch to my tree and will push along with my other
ixgbe patches to dave/netdev. Thanks.
--
Cheers,
Jeff
^ permalink raw reply
* Re: [PATCH net-next-2.6] ixgbe: correct the parameter description
From: Jeff Kirsher @ 2009-09-24 19:08 UTC (permalink / raw)
To: Peter P Waskiewicz Jr
Cc: Jiri Pirko, davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <1253816772.3153.2.camel@localhost.localdomain>
On Thu, Sep 24, 2009 at 11:26, Peter P Waskiewicz Jr
<peter.p.waskiewicz.jr@intel.com> wrote:
> On Thu, 2009-09-24 at 07:36 -0700, Jiri Pirko wrote:
>> ccffad25b5136958d4769ed6de5e87992dd9c65c changed parameters for function
>> ixgbe_update_uc_addr_list_generic but parameter description was not updated.
>> This patch corrects it.
>>
>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>
> Thanks Jiri,
>
> Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>
Jiri/Dave-
I already have a few ixgbe patches in my queue, so I will add this
patch to my queue and push it along with my other patches to
Dave/netdev.
--
Cheers,
Jeff
^ permalink raw reply
* [PATCH] inet_peer: Optimize inet_getid()
From: Eric Dumazet @ 2009-09-24 19:04 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
While investigating for network latencies, I found inet_getid() was a contention point
for some workloads.
Fix is straightforward, using cmpxchg() instead of
a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
Another possibility was to use an atomic_t and atomic_add_return() but
the size of struct inet_peer object would had doubled on x86_64 because of
SLAB_HWCACHE_ALIGN constraint.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/inetpeer.h | 16 ++++++++--------
net/ipv4/inetpeer.c | 3 ---
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index 15e1f8f..952f0ad 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -37,17 +37,17 @@ struct inet_peer *inet_getpeer(__be32 daddr, int create);
/* can be called from BH context or outside */
extern void inet_putpeer(struct inet_peer *p);
-extern spinlock_t inet_peer_idlock;
/* can be called with or without local BH being disabled */
static inline __u16 inet_getid(struct inet_peer *p, int more)
{
- __u16 id;
-
- spin_lock_bh(&inet_peer_idlock);
- id = p->ip_id_count;
- p->ip_id_count += 1 + more;
- spin_unlock_bh(&inet_peer_idlock);
- return id;
+ __u16 old;
+
+ while (1) {
+ old = p->ip_id_count;
+ if (cmpxchg(&p->ip_id_count, old, old + 1 + more) == old)
+ break;
+ }
+ return old;
}
#endif /* _NET_INETPEER_H */
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index b1fbe18..5dc29b8 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -67,9 +67,6 @@
* ip_id_count: idlock
*/
-/* Exported for inet_getid inline function. */
-DEFINE_SPINLOCK(inet_peer_idlock);
-
static struct kmem_cache *peer_cachep __read_mostly;
#define node_height(x) x->avl_height
^ permalink raw reply related
* [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: Anton Vorontsov @ 2009-09-24 18:31 UTC (permalink / raw)
To: David Miller; +Cc: Rafael J. Wysocki, linux-pm, netdev
Following trace pops up if we try to suspend with 3c59x ethernet NIC
brought down:
root@b1:~# ifconfig eth16 down
root@b1:~# echo mem > /sys/power/state
...
3c59x 0000:00:10.0: suspend
3c59x 0000:00:10.0: PME# disabled
Trying to free already-free IRQ 48
------------[ cut here ]------------
Badness at c00554e4 [verbose debug info unavailable]
NIP: c00554e4 LR: c00554e4 CTR: c019a098
REGS: c7975c60 TRAP: 0700 Not tainted (2.6.31-rc4)
MSR: 00021032 <ME,CE,IR,DR> CR: 28242422 XER: 20000000
TASK = c79cb0c0[1746] 'bash' THREAD: c7974000
...
NIP [c00554e4] __free_irq+0x108/0x1b0
LR [c00554e4] __free_irq+0x108/0x1b0
Call Trace:
[c7975d10] [c00554e4] __free_irq+0x108/0x1b0 (unreliable)
[c7975d30] [c005559c] free_irq+0x10/0x24
[c7975d40] [c01e21ec] vortex_suspend+0x70/0xc4
[c7975d60] [c017e584] pci_legacy_suspend+0x58/0x100
This is because the driver manages interrupts without checking for
netif_running().
Though, there are few other issues with suspend/resume in this driver.
The intention of calling free_irq() in suspend() was to avoid any
possible spurious interrupts (see commit 5b039e681b8c5f30aac9cc04385
"3c59x PM fixes"). But,
- On resume, the driver was requesting IRQ just after pci_set_master(),
but before vortex_up() (which actually resets 3c59x chips).
- Issuing free_irq() on a shared IRQ doesn't guarantee that a buggy
HW won't trigger spurious interrupts in another driver that
requested the same interrupt. So, if we want to protect from
unexpected interrupts, then on suspend we should issue disable_irq(),
not free_irq().
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
drivers/net/3c59x.c | 12 +++---------
1 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index c34aee9..7cdd4b0 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -807,10 +807,10 @@ static int vortex_suspend(struct pci_dev *pdev, pm_message_t state)
if (netif_running(dev)) {
netif_device_detach(dev);
vortex_down(dev, 1);
+ disable_irq(dev->irq);
}
pci_save_state(pdev);
pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
- free_irq(dev->irq, dev);
pci_disable_device(pdev);
pci_set_power_state(pdev, pci_choose_state(pdev, state));
}
@@ -833,18 +833,12 @@ static int vortex_resume(struct pci_dev *pdev)
return err;
}
pci_set_master(pdev);
- if (request_irq(dev->irq, vp->full_bus_master_rx ?
- &boomerang_interrupt : &vortex_interrupt, IRQF_SHARED, dev->name, dev)) {
- pr_warning("%s: Could not reserve IRQ %d\n", dev->name, dev->irq);
- pci_disable_device(pdev);
- return -EBUSY;
- }
if (netif_running(dev)) {
err = vortex_up(dev);
if (err)
return err;
- else
- netif_device_attach(dev);
+ enable_irq(dev->irq);
+ netif_device_attach(dev);
}
}
return 0;
--
1.6.3.3
^ permalink raw reply related
* Re: ixgbe patch to provide NIC's tx/rx counters via ethtool
From: Peter P Waskiewicz Jr @ 2009-09-24 18:28 UTC (permalink / raw)
To: Ben Greear; +Cc: NetDev
In-Reply-To: <4ABAA2D0.4030608@candelatech.com>
On Wed, 2009-09-23 at 15:36 -0700, Ben Greear wrote:
> When LRO is enabled, the received packet and byte counters represent the
> LRO'd packets, not the packets/bytes on the wire. The Intel 82599 NIC has
> registers that keep count of the physical packets. Add these counters to
> the ethtool stats. The byte counters are 36-bit, but the high 4 bits were
> being ignored in the 2.6.31 ixgbe driver: Read those as well to allow
> longer time between polling the stats to detect wraps.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
>
>
> Please do not apply this until the ixgbe authors ACK it. There may
> have been reasons for not reading the high 4 bits, or they may dislike
> this approach entirely.
Aside from the trivial line-wrap on the comments, I'm fine with this
patch. There is no issue I could find with the hardware that would
limit you from reading the high 4 bits. And since we're reading it
already to clear the register, we might as well use the value we get
from it.
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
^ permalink raw reply
* Re: [PATCH net-next-2.6] ixgbe: correct the parameter description
From: Peter P Waskiewicz Jr @ 2009-09-24 18:26 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <20090924143627.GD2919@psychotron.redhat.com>
On Thu, 2009-09-24 at 07:36 -0700, Jiri Pirko wrote:
> ccffad25b5136958d4769ed6de5e87992dd9c65c changed parameters for function
> ixgbe_update_uc_addr_list_generic but parameter description was not updated.
> This patch corrects it.
>
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
Thanks Jiri,
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
^ permalink raw reply
* Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
From: Gregory Haskins @ 2009-09-24 18:04 UTC (permalink / raw)
To: Avi Kivity
Cc: Ira W. Snyder, Michael S. Tsirkin, netdev, virtualization, kvm,
linux-kernel, mingo, linux-mm, akpm, hpa, Rusty Russell, s.hetze,
alacrityvm-devel
In-Reply-To: <4ABB27B9.4050904@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]
Avi Kivity wrote:
> On 09/23/2009 10:37 PM, Avi Kivity wrote:
>>
>> Example: feature negotiation. If it happens in userspace, it's easy
>> to limit what features we expose to the guest. If it happens in the
>> kernel, we need to add an interface to let the kernel know which
>> features it should expose to the guest. We also need to add an
>> interface to let userspace know which features were negotiated, if we
>> want to implement live migration. Something fairly trivial bloats
>> rapidly.
>
> btw, we have this issue with kvm reporting cpuid bits to the guest.
> Instead of letting kvm talk directly to the hardware and the guest, kvm
> gets the cpuid bits from the hardware, strips away features it doesn't
> support, exposes that to userspace, and expects userspace to program the
> cpuid bits it wants to expose to the guest (which may be different than
> what kvm exposed to userspace, and different from guest to guest).
>
This issue doesn't exist in the model I am referring to, as these are
all virtual-devices anyway. See my last reply
-Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
From: Gregory Haskins @ 2009-09-24 18:03 UTC (permalink / raw)
To: Avi Kivity
Cc: Ira W. Snyder, Michael S. Tsirkin, netdev, virtualization, kvm,
linux-kernel, mingo, linux-mm, akpm, hpa, Rusty Russell, s.hetze,
alacrityvm-devel
In-Reply-To: <4ABB1D44.5000007@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 24877 bytes --]
Avi Kivity wrote:
> On 09/24/2009 12:15 AM, Gregory Haskins wrote:
>>
>>>> There are various aspects about designing high-performance virtual
>>>> devices such as providing the shortest paths possible between the
>>>> physical resources and the consumers. Conversely, we also need to
>>>> ensure that we meet proper isolation/protection guarantees at the same
>>>> time. What this means is there are various aspects to any
>>>> high-performance PV design that require to be placed in-kernel to
>>>> maximize the performance yet properly isolate the guest.
>>>>
>>>> For instance, you are required to have your signal-path (interrupts and
>>>> hypercalls), your memory-path (gpa translation), and
>>>> addressing/isolation model in-kernel to maximize performance.
>>>>
>>>>
>>> Exactly. That's what vhost puts into the kernel and nothing more.
>>>
>> Actually, no. Generally, _KVM_ puts those things into the kernel, and
>> vhost consumes them. Without KVM (or something equivalent), vhost is
>> incomplete. One of my goals with vbus is to generalize the "something
>> equivalent" part here.
>>
>
> I don't really see how vhost and vbus are different here. vhost expects
> signalling to happen through a couple of eventfds and requires someone
> to supply them and implement kernel support (if needed). vbus requires
> someone to write a connector to provide the signalling implementation.
> Neither will work out-of-the-box when implementing virtio-net over
> falling dominos, for example.
I realize in retrospect that my choice of words above implies vbus _is_
complete, but this is not what I was saying. What I was trying to
convey is that vbus is _more_ complete. Yes, in either case some kind
of glue needs to be written. The difference is that vbus implements
more of the glue generally, and leaves less required to be customized
for each iteration.
Going back to our stack diagrams, you could think of a vhost solution
like this:
--------------------------
| virtio-net
--------------------------
| virtio-ring
--------------------------
| virtio-bus
--------------------------
| ? undefined-1 ?
--------------------------
| vhost
--------------------------
and you could think of a vbus solution like this
--------------------------
| virtio-net
--------------------------
| virtio-ring
--------------------------
| virtio-bus
--------------------------
| bus-interface
--------------------------
| ? undefined-2 ?
--------------------------
| bus-model
--------------------------
| virtio-net-device (vhost ported to vbus model? :)
--------------------------
So the difference between vhost and vbus in this particular context is
that you need to have "undefined-1" do device discovery/hotswap,
config-space, address-decode/isolation, signal-path routing, memory-path
routing, etc. Today this function is filled by things like virtio-pci,
pci-bus, KVM/ioeventfd, and QEMU for x86. I am not as familiar with
lguest, but presumably it is filled there by components like
virtio-lguest, lguest-bus, lguest.ko, and lguest-launcher. And to use
more contemporary examples, we might have virtio-domino, domino-bus,
domino.ko, and domino-launcher as well as virtio-ira, ira-bus, ira.ko,
and ira-launcher.
Contrast this to the vbus stack: The bus-X components (when optionally
employed by the connector designer) do device-discovery, hotswap,
config-space, address-decode/isolation, signal-path and memory-path
routing, etc in a general (and pv-centric) way. The "undefined-2"
portion is the "connector", and just needs to convey messages like
"DEVCALL" and "SHMSIGNAL". The rest is handled in other parts of the stack.
So to answer your question, the difference is that the part that has to
be customized in vbus should be a fraction of what needs to be
customized with vhost because it defines more of the stack. And, as
eluded to in my diagram, both virtio-net and vhost (with some
modifications to fit into the vbus framework) are potentially
complementary, not competitors.
>
>>>> Vbus accomplishes its in-kernel isolation model by providing a
>>>> "container" concept, where objects are placed into this container by
>>>> userspace. The host kernel enforces isolation/protection by using a
>>>> namespace to identify objects that is only relevant within a specific
>>>> container's context (namely, a "u32 dev-id"). The guest addresses the
>>>> objects by its dev-id, and the kernel ensures that the guest can't
>>>> access objects outside of its dev-id namespace.
>>>>
>>>>
>>> vhost manages to accomplish this without any kernel support.
>>>
>> No, vhost manages to accomplish this because of KVMs kernel support
>> (ioeventfd, etc). Without a KVM-like in-kernel support, vhost is a
>> merely a kind of "tuntap"-like clone signalled by eventfds.
>>
>
> Without a vbus-connector-falling-dominos, vbus-venet can't do anything
> either.
Mostly covered above...
However, I was addressing your assertion that vhost somehow magically
accomplishes this "container/addressing" function without any specific
kernel support. This is incorrect. I contend that this kernel support
is required and present. The difference is that its defined elsewhere
(and typically in a transport/arch specific way).
IOW: You can basically think of the programmed PIO addresses as forming
its "container". Only addresses explicitly added are visible, and
everything else is inaccessible. This whole discussion is merely a
question of what's been generalized verses what needs to be
re-implemented each time.
> Both vhost and vbus need an interface,
Agreed
> vhost's is just narrower since it doesn't do configuration or enumeration.
I would say that makes vhost solution's interface wider, not narrower.
With the vbus kvm-connector, simple vbus device instantiation implicitly
registers it in the address/enumeration namespace, and transmits a
devadd event. It does all that with no more interface complexity than
instantiating a vhost device. However, vhost has to then also
separately configure its address/enumeration space with other subsystems
(e.g. pci, ioeventfd, msi, etc), and define its config-space twice.
This means something in userspace has to proxy and/or refactor requests.
This also means that the userspace component has to have some knowledge
of _how_ to proxy/refactor said requests (i.e. splitting the design),
which is another example of where the current vhost model really falls
apart IMO.
>
>> This goes directly to my rebuttal of your claim that vbus places too
>> much in the kernel. I state that, one way or the other, address decode
>> and isolation _must_ be in the kernel for performance. Vbus does this
>> with a devid/container scheme. vhost+virtio-pci+kvm does it with
>> pci+pio+ioeventfd.
>>
>
> vbus doesn't do kvm guest address decoding for the fast path. It's
> still done by ioeventfd.
That is not correct. vbus does its own native address decoding in the
fast path, such as here:
http://git.kernel.org/?p=linux/kernel/git/ghaskins/alacrityvm/linux-2.6.git;a=blob;f=kernel/vbus/client.c;h=e85b2d92d629734866496b67455dd307486e394a;hb=e6cbd4d1decca8e829db3b2b9b6ec65330b379e9#l331
The connector delivers a SHMSIGNAL(id) message, and its decoded
generically by an rcu protected radix tree.
I think what you are thinking of is that my KVM-connector in AlacrityVM
uses PIO/ioeventfd (*) as part of its transport to deliver that
SHMSIGNAL message. In this sense, I am doing two address-decodes (one
for the initial pio, one for the subsequent shmsignal), but this is an
implementation detail of the KVM connector.
(Also note that its an implementation detail that the KVM maintainer
forced me into ;) The original vbus design utilized a global hypercall
in place of the PIO, and thus the shmsignal was the only real decode
occurring)
(*) actually I dropped ioeventfd in my latest tree, but this is a
separate topic. I still use KVM's pio-bus, however.
>
>>> The guest
>>> simply has not access to any vhost resources other than the guest->host
>>> doorbell, which is handed to the guest outside vhost (so it's somebody
>>> else's problem, in userspace).
>>>
>> You mean _controlled_ by userspace, right? Obviously, the other side of
>> the kernel still needs to be programmed (ioeventfd, etc). Otherwise,
>> vhost would be pointless: e.g. just use vanilla tuntap if you don't need
>> fast in-kernel decoding.
>>
>
> Yes (though for something like level-triggered interrupts we're probably
> keeping it in userspace, enjoying the benefits of vhost data path while
> paying more for signalling).
Thats fine. I am primarily interested in the high-performance IO, so
low-perf/legacy components can fall back to something else like
userspace if that best serves them.
>
>>>> All that is required is a way to transport a message with a "devid"
>>>> attribute as an address (such as DEVCALL(devid)) and the framework
>>>> provides the rest of the decode+execute function.
>>>>
>>>>
>>> vhost avoids that.
>>>
>> No, it doesn't avoid it. It just doesn't specify how its done, and
>> relies on something else to do it on its behalf.
>>
>
> That someone else can be in userspace, apart from the actual fast path.
No, this "devcall" like decoding _is_ fast path and it can't be in
userspace if you care about performance. And if you don't care about
performance, you can use existing facilities (like QEMU+tuntap) so vhost
and vbus alike would become unnecessary in that scenario.
>
>> Conversely, vbus specifies how its done, but not how to transport the
>> verb "across the wire". That is the role of the vbus-connector
>> abstraction.
>>
>
> So again, vbus does everything in the kernel (since it's so easy and
> cheap) but expects a vbus-connector. vhost does configuration in
> userspace (since it's so clunky and fragile) but expects a couple of
> eventfds.
Well, we are talking about fast-path here, so I am not sure why
config-space is coming up in this context. I digress. I realize you are
being sarcastic, but your easy+cheap/clunky+fragile assessment is more
accurate than you perhaps realize.
You keep extolling that vhost does most things in userspace and that is
an advantage. But the simple fact is that they both functionally do
almost the same amount in-kernel, because they _have_ to. This includes
the obvious stuff like signal and memory routing, but also the less
obvious stuff like most of config-space. Ultimately, most of
config-space needs to terminate at the device-model (the one exception
is perhaps "read-only attributes", like "MACQUERY"). Therefore, even if
you use a vhost like model, most of your parameters will invariably be a
translation from one config space to another and passed on (e.g. pci
config-cycle to ioctl()).
The disparity of in-kernel vs userspace functionality that remains
between the two implementations are basically the enumeration/hotswap
and read-only attribute functions. These functions are prohibitively
complex in the vhost+virtio-pci+kvm model (full ICH/pci chipset
emulation, etc), so I understand why we wouldnt want to move those
in-kernel. However, vbus was designed from scratch specifically for PV
to be flexible and simple. As a result, the remaining functions in the
kvm-connector take advantage of this simplicity and just ride on the
existing model that we needed for fast-path anyway. What this means is
there are of no significant consequence to do these few minor details
in-kernel, other than this long discussion.
In fact, it's actually a simpler design to unify things this way because
you avoid splitting the device model up. Consider how painful the vhost
implementation would be if it didn't already have the userspace
virtio-net to fall-back on. This is effectively what we face for new
devices going forward if that model is to persist.
>
>>>> Contrast this to vhost+virtio-pci (called simply "vhost" from here).
>>>>
>>>>
>>> It's the wrong name. vhost implements only the data path.
>>>
>> Understood, but vhost+virtio-pci is what I am contrasting, and I use
>> "vhost" for short from that point on because I am too lazy to type the
>> whole name over and over ;)
>>
>
> If you #define A A+B+C don't expect intelligent conversation afterwards.
Fair enough, but I did attempt to declare the definition before using
it. Sorry again for the confusion.
>
>>>> It is not immune to requiring in-kernel addressing support either, but
>>>> rather it just does it differently (and its not as you might expect via
>>>> qemu).
>>>>
>>>> Vhost relies on QEMU to render PCI objects to the guest, which the
>>>> guest
>>>> assigns resources (such as BARs, interrupts, etc).
>>>>
>>> vhost does not rely on qemu. It relies on its user to handle
>>> configuration. In one important case it's qemu+pci. It could just as
>>> well be the lguest launcher.
>>>
>> I meant vhost=vhost+virtio-pci here. Sorry for the confusion.
>>
>> The point I am making specifically is that vhost in general relies on
>> other in-kernel components to function. I.e. It cannot function without
>> having something like the PCI model to build an IO namespace. That
>> namespace (in this case, pio addresses+data tuples) are used for the
>> in-kernel addressing function under KVM + virtio-pci.
>>
>> The case of the lguest launcher is a good one to highlight. Yes, you
>> can presumably also use lguest with vhost, if the requisite facilities
>> are exposed to lguest-bus, and some eventfd based thing like ioeventfd
>> is written for the host (if it doesnt exist already).
>>
>> And when the next virt design "foo" comes out, it can make a "foo-bus"
>> model, and implement foo-eventfd on the backend, etc, etc.
>>
>
> It's exactly the same with vbus needing additional connectors for
> additional transports.
No, see my reply above.
>
>> Ira can make ira-bus, and ira-eventfd, etc, etc.
>>
>> Each iteration will invariably introduce duplicated parts of the stack.
>>
>
> Invariably?
As in "always"
> Use libraries (virtio-shmem.ko, libvhost.so).
What do you suppose vbus is? vbus-proxy.ko = virtio-shmem.ko, and you
dont need libvhost.so per se since you can just use standard kernel
interfaces (like configfs/sysfs). I could create an .so going forward
for the new ioctl-based interface, I suppose.
>
>
>>> For the N+1th time, no. vhost is perfectly usable without pci. Can we
>>> stop raising and debunking this point?
>>>
>> Again, I understand vhost is decoupled from PCI, and I don't mean to
>> imply anything different. I use PCI as an example here because a) its
>> the only working example of vhost today (to my knowledge), and b) you
>> have stated in the past that PCI is the only "right" way here, to
>> paraphrase. Perhaps you no longer feel that way, so I apologize if you
>> feel you already recanted your position on PCI and I missed it.
>>
>
> For kvm/x86 pci definitely remains king.
For full virtualization, sure. I agree. However, we are talking about
PV here. For PV, PCI is not a requirement and is a technical dead-end IMO.
KVM seems to be the only virt solution that thinks otherwise (*), but I
believe that is primarily a condition of its maturity. I aim to help
advance things here.
(*) citation: xen has xenbus, lguest has lguest-bus, vmware has some
vmi-esq thing (I forget what its called) to name a few. Love 'em or
hate 'em, most other hypervisors do something along these lines. I'd
like to try to create one for KVM, but to unify them all (at least for
the Linux-based host designs).
> I was talking about the two
> lguest users and Ira.
>
>> I digress. My point here isn't PCI. The point here is the missing
>> component for when PCI is not present. The component that is partially
>> satisfied by vbus's devid addressing scheme. If you are going to use
>> vhost, and you don't have PCI, you've gotta build something to replace
>> it.
>>
>
> Yes, that's why people have keyboards. They'll write that glue code if
> they need it. If it turns out to be a hit an people start having virtio
> transport module writing parties, they'll figure out a way to share code.
Sigh... The party has already started. I tried to invite you months ago...
>
>>>> All you really need is a simple decode+execute mechanism, and a way to
>>>> program it from userspace control. vbus tries to do just that:
>>>> commoditize it so all you need is the transport of the control messages
>>>> (like DEVCALL()), but the decode+execute itself is reuseable, even
>>>> across various environments (like KVM or Iras rig).
>>>>
>>>>
>>> If you think it should be "commodotized", write libvhostconfig.so.
>>>
>> I know you are probably being facetious here, but what do you propose
>> for the parts that must be in-kernel?
>>
>
> On the guest side, virtio-shmem.ko can unify the ring access. It
> probably makes sense even today. On the host side I eventfd is the
> kernel interface and libvhostconfig.so can provide the configuration
> when an existing ABI is not imposed.
That won't cut it. For one, creating an eventfd is only part of the
equation. I.e. you need to have originate/terminate somewhere
interesting (and in-kernel, otherwise use tuntap).
>
>>>> And your argument, I believe, is that vbus allows both to be
>>>> implemented
>>>> in the kernel (though to reiterate, its optional) and is therefore a
>>>> bad
>>>> design, so lets discuss that.
>>>>
>>>> I believe the assertion is that things like config-space are best left
>>>> to userspace, and we should only relegate fast-path duties to the
>>>> kernel. The problem is that, in my experience, a good deal of
>>>> config-space actually influences the fast-path and thus needs to
>>>> interact with the fast-path mechanism eventually anyway.
>>>> Whats left
>>>> over that doesn't fall into this category may cheaply ride on existing
>>>> plumbing, so its not like we created something new or unnatural just to
>>>> support this subclass of config-space.
>>>>
>>>>
>>> Flexibility is reduced, because changing code in the kernel is more
>>> expensive than in userspace, and kernel/user interfaces aren't typically
>>> as wide as pure userspace interfaces. Security is reduced, since a bug
>>> in the kernel affects the host, while a bug in userspace affects just on
>>> guest.
>>>
>> For a mac-address attribute? Thats all we are really talking about
>> here. These points you raise, while true of any kernel code I suppose,
>> are a bit of a stretch in this context.
>>
>
> Look at the virtio-net feature negotiation. There's a lot more there
> than the MAC address, and it's going to grow.
Agreed, but note that makes my point. That feature negotiation almost
invariably influences the device-model, not some config-space shim.
IOW: terminating config-space at some userspace shim is pointless. The
model ultimately needs the result of whatever transpires during that
negotiation anyway.
>
>>> Example: feature negotiation. If it happens in userspace, it's easy to
>>> limit what features we expose to the guest.
>>>
>> Its not any harder in the kernel. I do this today.
>>
>> And when you are done negotiating said features, you will generally have
>> to turn around and program the feature into the backend anyway (e.g.
>> ioctl() to vhost module). Now you have to maintain some knowledge of
>> that particular feature and how to program it in two places.
>>
>
> No, you can leave it enabled unconditionally in vhost (the guest won't
> use what it doesn't know about).
Perhaps, but IMO sending a "feature-mask"-like object down is far easier
then proxying/refactoring config-space and sending that down. I'd still
chalk the win here to the vbus model used in AlacrityVM.
FWIW: venet has the ability to enable/disable features on the host side,
so clearly userspace config-space is not required for the basic premise.
>
>> Conversely, I am eliminating the (unnecessary) middleman by letting the
>> feature negotiating take place directly between the two entities that
>> will consume it.
>>
>
> The middleman is necessary, if you want to support live migration
Orchestrating live-migration has nothing to do with whether config-space
is serviced by a middle-man or not. It shouldn't be required to have
device-specific knowledge at all beyond what was initially needed to
create/config the object at boot time.
IOW, the orchestrator merely needs to know that a device-model object is
present and a method to serialize and reconstitute its state (if
appropriate).
, or to
> restrict a guest to a subset of your features.
No, that is incorrect. We are not talking about directly exposing
something like HW cpuid here. These are all virtual models, and they
can optionally expose as much or as little as we want. They do this
under administrative control by userspace, and independent of the
location of the config-space handler.
>
>>> If it happens in the
>>> kernel, we need to add an interface to let the kernel know which
>>> features it should expose to the guest.
>>>
>> You need this already either way for both models anyway. As an added
>> bonus, vbus has generalized that interface using sysfs attributes, so
>> all models are handled in a similar and community accepted way.
>>
>
> vhost doesn't need it since userspace takes care of it.
Ok, but see my related reply above.
>
>>> We also need to add an
>>> interface to let userspace know which features were negotiated, if we
>>> want to implement live migration. Something fairly trivial bloats
>>> rapidly.
>>>
>> Can you elaborate on the requirements for live-migration? Wouldnt an
>> opaque save/restore model work here? (e.g. why does userspace need to be
>> able to interpret the in-kernel state, just pass it along as a blob to
>> the new instance).
>>
>
> A blob would work, if you commit to forward and backward compatibility
> in the kernel side (i.e. an older kernel must be able to accept a blob
> from a newer one).
Thats understood and acceptable.
> I don't like blobs though, they tie you to the implemenetation.
What would you suggest otherwise?
>
>>> As you can see above, userspace needs to be involved in this, and the
>>> number of interfaces required is smaller if it's in userspace:
>>>
>> Actually, no. My experience has been the opposite. Anytime I sat down
>> and tried to satisfy your request to move things to the userspace,
>> things got ugly and duplicative really quick. I suspect part of the
>> reason you may think its easier because you already have part of
>> virtio-net in userspace and its surrounding support, but that is not the
>> case moving forward for new device types.
>>
>
> I can't comment on your experience, but we'll definitely build on
> existing code for new device types.
Fair enough. I'll build on my experience, either reusing existing code
or implementing new designs where appropriate. If you or anyone else
want to join me in my efforts, the more the merrier.
>
>>> you only
>>> need to know which features the kernel supports (they can be enabled
>>> unconditionally, just not exposed).
>>>
>>> Further, some devices are perfectly happy to be implemented in
>>> userspace, so we need userspace configuration support anyway. Why
>>> reimplement it in the kernel?
>>>
>> Thats fine. vbus is targetted for high-performance IO. So if you have
>> a robust userspace (like KVM+QEMU) and low-performance constraints (say,
>> for a console or something), put it in userspace and vbus is not
>> involved. I don't care.
>>
>
> So now the hypothetical non-pci hypervisor needs to support two busses.
No. The hypothetical hypervisor only needs to decide where
low-performance devices should live. If that is best served by
making/reusing a unique bus for them, I have no specific problem with
that. Systems are typically composed of multiple buses anyway.
Conversely, there is nothing wrong with putting low-performance devices
on a bus designed for high-performance either, and vbus can accommodate
both types. The latter is what I would advocate for simplicity's sake,
but its not a requirement.
Kind Regards,
-Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* question on raw sockets and source IP address validation
From: Chris Friesen @ 2009-09-24 17:53 UTC (permalink / raw)
To: Linux Network Development list
Hi all,
Normally when sending a packet on a SOCK_RAW socket the source IP
address is validated against the addresses configured on the host. If
the address isn't configured, the packet isn't sent.
This can be avoided by setting IP_HDRINCL, but then the app needs to
handle all the fragmentation itself.
Is there any way to bypass the source address validation without IP_HDRINCL?
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH 3/3 V2] i2400m-sdio: select IWMC3200TOP in Kconfig
From: Inaky Perez-Gonzalez @ 2009-09-24 17:25 UTC (permalink / raw)
To: Winkler, Tomas
Cc: davem@davemloft.net, linville@tuxdriver.com,
netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
linux-mmc@vger.kernel.org, Zhu, Yi, Kao, Cindy H, Cohen, Guy,
Rindjunsky, Ron
In-Reply-To: <1253779256-17058-1-git-send-email-tomas.winkler@intel.com>
On Thu, 2009-09-24 at 02:00 -0600, Winkler, Tomas wrote:
> i2400m-sdio requires iwmc3200top for its operation
>
> create separate config option to separate 3200 specifics
> from eventual further wimax sdio HW.
>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Acked-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
I'll merge this into the WiMAX tree.
--
-- Inaky
^ permalink raw reply
* Re: ixgbe patch to provide NIC's tx/rx counters via ethtool
From: Ben Greear @ 2009-09-24 17:07 UTC (permalink / raw)
To: Rick Jones; +Cc: NetDev
In-Reply-To: <4ABB9EB3.1000307@hp.com>
On 09/24/2009 09:30 AM, Rick Jones wrote:
> Ben Greear wrote:
>> Rick Jones wrote:
>>> Ben Greear wrote:
>>>
>>>> When LRO is enabled, the received packet and byte counters represent
>>>> the
>>>> LRO'd packets, not the packets/bytes on the wire.
>>>
>>>
>>> When LRO is enabled, are all the bytes on the wire actually
>>> transferred into the host?
>>
>> No...the ethernet, IP and TCP headers and such are not, for packets
>> that are combined into a single large SKB.
>>
>> That is why the driver counts them wrong. The bytes are off by a few
>> percentage points, but the packet count is off by an order of magnitude.
>
> An overly philosphical question perhaps, but are ethtool stats supposed
> to represent what was on the wire, or what entered the host?
They report whatever they report, you get to set custom labels for the values,
and every NIC/driver may be different, so only humans and crazy code like mine that does
specific things based on the driver reported by ethtool should use it.
A more interesting question to me is what netdev-stats tx/rx byte counters should report?
My opinions:
ethernet header (yes)
ethernet CRC (yes)
ethernet preamble (no)
ethernet frame gap (no)
I think many don't count the CRC, but I haven't looked recently.
Some didn't even report the ethernet header properly a few years ago, but
I think most do now.
When LRO is enabled, it's hard to say if we should report the LRO pkt
stats or the stats on the wire for the netdev-stats. At least in my case,
I want to report the stats on the wire, but it's also good to see the
LRO stats because you can easily tell that LRO is actually working if you
see low pkts-per-second counters v/s high-bits-per-sec.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] net: fix htmldocs sunrpc, clnt.c
From: Benny Halevy @ 2009-09-24 16:46 UTC (permalink / raw)
To: Randy Dunlap, Jaswinder Singh Rajput
Cc: Ricardo Labiaga, Andy Adamson, Trond Myklebust, David Miller,
linux-nfs, netdev, LKML
In-Reply-To: <4ABB9C17.3020307@oracle.com>
On Sep. 24, 2009, 19:19 +0300, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> Jaswinder Singh Rajput wrote:
>> DOCPROC Documentation/DocBook/networking.xml
>> Warning(net/sunrpc/clnt.c:647): No description found for parameter 'req'
>> Warning(net/sunrpc/clnt.c:647): No description found for parameter 'tk_ops'
>> Warning(net/sunrpc/clnt.c:647): Excess function parameter 'ops' description in 'rpc_run_bc_task'
>>
>> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
>
> Ack. Already sent, but possibly lost.
Ack. thanks!
>
>> Cc: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
>> Cc: Benny Halevy <bhalevy@panasas.com>
>> Cc: Andy Adamson <andros@netapp.com>
>> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
>> Cc: Randy Dunlap <randy.dunlap@oracle.com>
>> Cc: David Miller <davem@davemloft.net>
>> ---
>> net/sunrpc/clnt.c | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index a417d5a..38829e2 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -640,10 +640,11 @@ EXPORT_SYMBOL_GPL(rpc_call_async);
>> /**
>> * rpc_run_bc_task - Allocate a new RPC task for backchannel use, then run
>> * rpc_execute against it
>> - * @ops: RPC call ops
>> + * @req: RPC request
>> + * @tk_ops: RPC call ops
>> */
>> struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
>> - const struct rpc_call_ops *tk_ops)
>> + const struct rpc_call_ops *tk_ops)
>> {
>> struct rpc_task *task;
>> struct xdr_buf *xbufp = &req->rq_snd_buf;
>
^ 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