* [PATCH net-next 0/5] RDS: minor updates for net-next
From: Andy Grover @ 2009-10-30 18:54 UTC (permalink / raw)
To: netdev; +Cc: rds-devel
These patches fix some small issues with RDS, please review and
apply if ok.
Thanks -- Regards -- Andy
^ permalink raw reply
* [PATCH 1/5] RDS: Add GET_MR_FOR_DEST sockopt
From: Andy Grover @ 2009-10-30 18:54 UTC (permalink / raw)
To: netdev; +Cc: rds-devel
In-Reply-To: <1256928893-17844-1-git-send-email-andy.grover@oracle.com>
RDS currently supports a GET_MR sockopt to establish a
memory region (MR) for a chunk of memory. However, the fastreg
method ties a MR to a particular destination. The GET_MR_FOR_DEST
sockopt allows the remote machine to be specified, and thus
support for fastreg (aka FRWRs).
Note that this patch does *not* do all of this - it simply
implements the new sockopt in terms of the old one, so applications
can begin to use the new sockopt in preparation for cutover to
FRWRs.
Signed-off-by: Andy Grover <andy.grover@oracle.com>
---
include/linux/rds.h | 8 ++++++++
net/rds/af_rds.c | 3 +++
net/rds/rdma.c | 24 ++++++++++++++++++++++++
net/rds/rdma.h | 1 +
4 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/include/linux/rds.h b/include/linux/rds.h
index 89d46e1..cab4994 100644
--- a/include/linux/rds.h
+++ b/include/linux/rds.h
@@ -56,6 +56,7 @@
/* deprecated: RDS_BARRIER 4 */
#define RDS_RECVERR 5
#define RDS_CONG_MONITOR 6
+#define RDS_GET_MR_FOR_DEST 7
/*
* Control message types for SOL_RDS.
@@ -224,6 +225,13 @@ struct rds_get_mr_args {
uint64_t flags;
};
+struct rds_get_mr_for_dest_args {
+ struct sockaddr_storage dest_addr;
+ struct rds_iovec vec;
+ u_int64_t cookie_addr;
+ uint64_t flags;
+};
+
struct rds_free_mr_args {
rds_rdma_cookie_t cookie;
u_int64_t flags;
diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index a202e5b..2b978dc 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c
@@ -265,6 +265,9 @@ static int rds_setsockopt(struct socket *sock, int level, int optname,
case RDS_GET_MR:
ret = rds_get_mr(rs, optval, optlen);
break;
+ case RDS_GET_MR_FOR_DEST:
+ ret = rds_get_mr_for_dest(rs, optval, optlen);
+ break;
case RDS_FREE_MR:
ret = rds_free_mr(rs, optval, optlen);
break;
diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index 8dc83d2..971b5a6 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -317,6 +317,30 @@ int rds_get_mr(struct rds_sock *rs, char __user *optval, int optlen)
return __rds_rdma_map(rs, &args, NULL, NULL);
}
+int rds_get_mr_for_dest(struct rds_sock *rs, char __user *optval, int optlen)
+{
+ struct rds_get_mr_for_dest_args args;
+ struct rds_get_mr_args new_args;
+
+ if (optlen != sizeof(struct rds_get_mr_for_dest_args))
+ return -EINVAL;
+
+ if (copy_from_user(&args, (struct rds_get_mr_for_dest_args __user *)optval,
+ sizeof(struct rds_get_mr_for_dest_args)))
+ return -EFAULT;
+
+ /*
+ * Initially, just behave like get_mr().
+ * TODO: Implement get_mr as wrapper around this
+ * and deprecate it.
+ */
+ new_args.vec = args.vec;
+ new_args.cookie_addr = args.cookie_addr;
+ new_args.flags = args.flags;
+
+ return __rds_rdma_map(rs, &new_args, NULL, NULL);
+}
+
/*
* Free the MR indicated by the given R_Key
*/
diff --git a/net/rds/rdma.h b/net/rds/rdma.h
index 4255120..909c398 100644
--- a/net/rds/rdma.h
+++ b/net/rds/rdma.h
@@ -61,6 +61,7 @@ static inline u32 rds_rdma_cookie_offset(rds_rdma_cookie_t cookie)
}
int rds_get_mr(struct rds_sock *rs, char __user *optval, int optlen);
+int rds_get_mr_for_dest(struct rds_sock *rs, char __user *optval, int optlen);
int rds_free_mr(struct rds_sock *rs, char __user *optval, int optlen);
void rds_rdma_drop_keys(struct rds_sock *rs);
int rds_cmsg_rdma_args(struct rds_sock *rs, struct rds_message *rm,
--
1.6.3.3
^ permalink raw reply related
* [PATCH 2/5] RDS: Fix potential race around rds_i[bw]_allocation
From: Andy Grover @ 2009-10-30 18:51 UTC (permalink / raw)
To: netdev; +Cc: rds-devel
In-Reply-To: <1256928717-17757-1-git-send-email-andy.grover@oracle.com>
>From Shin Hong:
"At rds_ib_recv_refill_one(), it first executes atomic_read(&rds_ib_allocation)
for if-condition checking,
and then executes atomic_inc(&rds_ib_allocation) if the condition was
not satisfied.
However, if any other code which updates rds_ib_allocation executes
between these two atomic operation executions,
it seems that it may result race condition. (especially when
rds_ib_allocation + 1 == rds_ib_sysctl_max_recv_allocation)"
This patch fixes this by using atomic_inc_unless to eliminate the
possibility of allocating more than rds_ib_sysctl_max_recv_allocation
and then decrementing the count if the allocation fails. It also
makes an identical change to the iwarp transport.
Reported-by: Shin Hong <hongshin@gmail.com>
Signed-off-by: Andy Grover <andy.grover@oracle.com>
---
net/rds/ib_recv.c | 7 ++++---
net/rds/iw_recv.c | 7 ++++---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index cd7a6cf..2f009d3 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -143,15 +143,16 @@ static int rds_ib_recv_refill_one(struct rds_connection *conn,
int ret = -ENOMEM;
if (recv->r_ibinc == NULL) {
- if (atomic_read(&rds_ib_allocation) >= rds_ib_sysctl_max_recv_allocation) {
+ if (!atomic_add_unless(&rds_ib_allocation, 1, rds_ib_sysctl_max_recv_allocation)) {
rds_ib_stats_inc(s_ib_rx_alloc_limit);
goto out;
}
recv->r_ibinc = kmem_cache_alloc(rds_ib_incoming_slab,
kptr_gfp);
- if (recv->r_ibinc == NULL)
+ if (recv->r_ibinc == NULL) {
+ atomic_dec(&rds_ib_allocation);
goto out;
- atomic_inc(&rds_ib_allocation);
+ }
INIT_LIST_HEAD(&recv->r_ibinc->ii_frags);
rds_inc_init(&recv->r_ibinc->ii_inc, conn, conn->c_faddr);
}
diff --git a/net/rds/iw_recv.c b/net/rds/iw_recv.c
index 8683f5f..9f98150 100644
--- a/net/rds/iw_recv.c
+++ b/net/rds/iw_recv.c
@@ -143,15 +143,16 @@ static int rds_iw_recv_refill_one(struct rds_connection *conn,
int ret = -ENOMEM;
if (recv->r_iwinc == NULL) {
- if (atomic_read(&rds_iw_allocation) >= rds_iw_sysctl_max_recv_allocation) {
+ if (!atomic_add_unless(&rds_iw_allocation, 1, rds_iw_sysctl_max_recv_allocation)) {
rds_iw_stats_inc(s_iw_rx_alloc_limit);
goto out;
}
recv->r_iwinc = kmem_cache_alloc(rds_iw_incoming_slab,
kptr_gfp);
- if (recv->r_iwinc == NULL)
+ if (recv->r_iwinc == NULL) {
+ atomic_dec(&rds_iw_allocation);
goto out;
- atomic_inc(&rds_iw_allocation);
+ }
INIT_LIST_HEAD(&recv->r_iwinc->ii_frags);
rds_inc_init(&recv->r_iwinc->ii_inc, conn, conn->c_faddr);
}
--
1.6.3.3
^ permalink raw reply related
* [PATCH 5/5] RDS/IB+IW: Move recv processing to a tasklet
From: Andy Grover @ 2009-10-30 18:51 UTC (permalink / raw)
To: netdev; +Cc: rds-devel
In-Reply-To: <1256928717-17757-1-git-send-email-andy.grover@oracle.com>
Move receive processing from event handler to a tasklet.
This should help prevent hangcheck timer from going off
when RDS is under heavy load.
Signed-off-by: Andy Grover <andy.grover@oracle.com>
---
net/rds/ib.h | 2 ++
net/rds/ib_cm.c | 2 ++
net/rds/ib_recv.c | 28 ++++++++++++++++++++++------
net/rds/iw.h | 2 ++
net/rds/iw_cm.c | 2 ++
net/rds/iw_recv.c | 28 ++++++++++++++++++++++------
6 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/net/rds/ib.h b/net/rds/ib.h
index 1378b85..64df4e7 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -98,6 +98,7 @@ struct rds_ib_connection {
struct rds_ib_send_work *i_sends;
/* rx */
+ struct tasklet_struct i_recv_tasklet;
struct mutex i_recv_mutex;
struct rds_ib_work_ring i_recv_ring;
struct rds_ib_incoming *i_ibinc;
@@ -303,6 +304,7 @@ void rds_ib_inc_free(struct rds_incoming *inc);
int rds_ib_inc_copy_to_user(struct rds_incoming *inc, struct iovec *iov,
size_t size);
void rds_ib_recv_cq_comp_handler(struct ib_cq *cq, void *context);
+void rds_ib_recv_tasklet_fn(unsigned long data);
void rds_ib_recv_init_ring(struct rds_ib_connection *ic);
void rds_ib_recv_clear_ring(struct rds_ib_connection *ic);
void rds_ib_recv_init_ack(struct rds_ib_connection *ic);
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index c2d372f..9d32069 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -694,6 +694,8 @@ int rds_ib_conn_alloc(struct rds_connection *conn, gfp_t gfp)
return -ENOMEM;
INIT_LIST_HEAD(&ic->ib_node);
+ tasklet_init(&ic->i_recv_tasklet, rds_ib_recv_tasklet_fn,
+ (unsigned long) ic);
mutex_init(&ic->i_recv_mutex);
#ifndef KERNEL_HAS_ATOMIC64
spin_lock_init(&ic->i_ack_lock);
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 2f009d3..fe5ab8c 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -825,17 +825,22 @@ void rds_ib_recv_cq_comp_handler(struct ib_cq *cq, void *context)
{
struct rds_connection *conn = context;
struct rds_ib_connection *ic = conn->c_transport_data;
- struct ib_wc wc;
- struct rds_ib_ack_state state = { 0, };
- struct rds_ib_recv_work *recv;
rdsdebug("conn %p cq %p\n", conn, cq);
rds_ib_stats_inc(s_ib_rx_cq_call);
- ib_req_notify_cq(cq, IB_CQ_SOLICITED);
+ tasklet_schedule(&ic->i_recv_tasklet);
+}
- while (ib_poll_cq(cq, 1, &wc) > 0) {
+static inline void rds_poll_cq(struct rds_ib_connection *ic,
+ struct rds_ib_ack_state *state)
+{
+ struct rds_connection *conn = ic->conn;
+ struct ib_wc wc;
+ struct rds_ib_recv_work *recv;
+
+ while (ib_poll_cq(ic->i_recv_cq, 1, &wc) > 0) {
rdsdebug("wc wr_id 0x%llx status %u byte_len %u imm_data %u\n",
(unsigned long long)wc.wr_id, wc.status, wc.byte_len,
be32_to_cpu(wc.ex.imm_data));
@@ -853,7 +858,7 @@ void rds_ib_recv_cq_comp_handler(struct ib_cq *cq, void *context)
if (rds_conn_up(conn) || rds_conn_connecting(conn)) {
/* We expect errors as the qp is drained during shutdown */
if (wc.status == IB_WC_SUCCESS) {
- rds_ib_process_recv(conn, recv, wc.byte_len, &state);
+ rds_ib_process_recv(conn, recv, wc.byte_len, state);
} else {
rds_ib_conn_error(conn, "recv completion on "
"%pI4 had status %u, disconnecting and "
@@ -864,6 +869,17 @@ void rds_ib_recv_cq_comp_handler(struct ib_cq *cq, void *context)
rds_ib_ring_free(&ic->i_recv_ring, 1);
}
+}
+
+void rds_ib_recv_tasklet_fn(unsigned long data)
+{
+ struct rds_ib_connection *ic = (struct rds_ib_connection *) data;
+ struct rds_connection *conn = ic->conn;
+ struct rds_ib_ack_state state = { 0, };
+
+ rds_poll_cq(ic, &state);
+ ib_req_notify_cq(ic->i_recv_cq, IB_CQ_SOLICITED);
+ rds_poll_cq(ic, &state);
if (state.ack_next_valid)
rds_ib_set_ack(ic, state.ack_next, state.ack_required);
diff --git a/net/rds/iw.h b/net/rds/iw.h
index dd72b62..eef2f0c 100644
--- a/net/rds/iw.h
+++ b/net/rds/iw.h
@@ -119,6 +119,7 @@ struct rds_iw_connection {
struct rds_iw_send_work *i_sends;
/* rx */
+ struct tasklet_struct i_recv_tasklet;
struct mutex i_recv_mutex;
struct rds_iw_work_ring i_recv_ring;
struct rds_iw_incoming *i_iwinc;
@@ -330,6 +331,7 @@ void rds_iw_inc_free(struct rds_incoming *inc);
int rds_iw_inc_copy_to_user(struct rds_incoming *inc, struct iovec *iov,
size_t size);
void rds_iw_recv_cq_comp_handler(struct ib_cq *cq, void *context);
+void rds_iw_recv_tasklet_fn(unsigned long data);
void rds_iw_recv_init_ring(struct rds_iw_connection *ic);
void rds_iw_recv_clear_ring(struct rds_iw_connection *ic);
void rds_iw_recv_init_ack(struct rds_iw_connection *ic);
diff --git a/net/rds/iw_cm.c b/net/rds/iw_cm.c
index a416b0d..394cf6b 100644
--- a/net/rds/iw_cm.c
+++ b/net/rds/iw_cm.c
@@ -696,6 +696,8 @@ int rds_iw_conn_alloc(struct rds_connection *conn, gfp_t gfp)
return -ENOMEM;
INIT_LIST_HEAD(&ic->iw_node);
+ tasklet_init(&ic->i_recv_tasklet, rds_iw_recv_tasklet_fn,
+ (unsigned long) ic);
mutex_init(&ic->i_recv_mutex);
#ifndef KERNEL_HAS_ATOMIC64
spin_lock_init(&ic->i_ack_lock);
diff --git a/net/rds/iw_recv.c b/net/rds/iw_recv.c
index 9f98150..24fc53f 100644
--- a/net/rds/iw_recv.c
+++ b/net/rds/iw_recv.c
@@ -784,17 +784,22 @@ void rds_iw_recv_cq_comp_handler(struct ib_cq *cq, void *context)
{
struct rds_connection *conn = context;
struct rds_iw_connection *ic = conn->c_transport_data;
- struct ib_wc wc;
- struct rds_iw_ack_state state = { 0, };
- struct rds_iw_recv_work *recv;
rdsdebug("conn %p cq %p\n", conn, cq);
rds_iw_stats_inc(s_iw_rx_cq_call);
- ib_req_notify_cq(cq, IB_CQ_SOLICITED);
+ tasklet_schedule(&ic->i_recv_tasklet);
+}
- while (ib_poll_cq(cq, 1, &wc) > 0) {
+static inline void rds_poll_cq(struct rds_iw_connection *ic,
+ struct rds_iw_ack_state *state)
+{
+ struct rds_connection *conn = ic->conn;
+ struct ib_wc wc;
+ struct rds_iw_recv_work *recv;
+
+ while (ib_poll_cq(ic->i_recv_cq, 1, &wc) > 0) {
rdsdebug("wc wr_id 0x%llx status %u byte_len %u imm_data %u\n",
(unsigned long long)wc.wr_id, wc.status, wc.byte_len,
be32_to_cpu(wc.ex.imm_data));
@@ -812,7 +817,7 @@ void rds_iw_recv_cq_comp_handler(struct ib_cq *cq, void *context)
if (rds_conn_up(conn) || rds_conn_connecting(conn)) {
/* We expect errors as the qp is drained during shutdown */
if (wc.status == IB_WC_SUCCESS) {
- rds_iw_process_recv(conn, recv, wc.byte_len, &state);
+ rds_iw_process_recv(conn, recv, wc.byte_len, state);
} else {
rds_iw_conn_error(conn, "recv completion on "
"%pI4 had status %u, disconnecting and "
@@ -823,6 +828,17 @@ void rds_iw_recv_cq_comp_handler(struct ib_cq *cq, void *context)
rds_iw_ring_free(&ic->i_recv_ring, 1);
}
+}
+
+void rds_iw_recv_tasklet_fn(unsigned long data)
+{
+ struct rds_iw_connection *ic = (struct rds_iw_connection *) data;
+ struct rds_connection *conn = ic->conn;
+ struct rds_iw_ack_state state = { 0, };
+
+ rds_poll_cq(ic, &state);
+ ib_req_notify_cq(ic->i_recv_cq, IB_CQ_SOLICITED);
+ rds_poll_cq(ic, &state);
if (state.ack_next_valid)
rds_iw_set_ack(ic, state.ack_next, state.ack_required);
--
1.6.3.3
^ permalink raw reply related
* [PATCH 4/5] RDS: Do not send congestion updates to loopback connections
From: Andy Grover @ 2009-10-30 18:51 UTC (permalink / raw)
To: netdev; +Cc: rds-devel
In-Reply-To: <1256928717-17757-1-git-send-email-andy.grover@oracle.com>
This issue was discovered by HP's Pradeep and fixed in OFED
1.3, but not fixed in later versions, since the fix's implementation
was not immediately applyable to the later code. This patch should
do the trick for 1.4+ codebases.
Signed-off-by: Andy Grover <andy.grover@oracle.com>
---
net/rds/cong.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/rds/cong.c b/net/rds/cong.c
index dd2711d..6d06cac 100644
--- a/net/rds/cong.c
+++ b/net/rds/cong.c
@@ -218,6 +218,8 @@ void rds_cong_queue_updates(struct rds_cong_map *map)
spin_lock_irqsave(&rds_cong_lock, flags);
list_for_each_entry(conn, &map->m_conn_list, c_map_item) {
+ if (conn->c_loopback)
+ continue;
if (!test_and_set_bit(0, &conn->c_map_queued)) {
rds_stats_inc(s_cong_update_queued);
queue_delayed_work(rds_wq, &conn->c_send_w, 0);
--
1.6.3.3
^ permalink raw reply related
* [PATCH 3/5] RDS: Fix panic on unload
From: Andy Grover @ 2009-10-30 18:51 UTC (permalink / raw)
To: netdev; +Cc: rds-devel
In-Reply-To: <1256928717-17757-1-git-send-email-andy.grover@oracle.com>
Remove explicit destruction of passive connection when destroying
active end of the connection. The passive end is also on the
device's connection list, and will thus be cleaned up properly.
Panic was caused by trying to clean it up twice.
Signed-off-by: Andy Grover <andy.grover@oracle.com>
---
net/rds/ib_rdma.c | 5 +----
net/rds/iw_rdma.c | 5 +----
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index ef3ab5b..c5e9165 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -187,11 +187,8 @@ void __rds_ib_destroy_conns(struct list_head *list, spinlock_t *list_lock)
INIT_LIST_HEAD(list);
spin_unlock_irq(list_lock);
- list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
- if (ic->conn->c_passive)
- rds_conn_destroy(ic->conn->c_passive);
+ list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
rds_conn_destroy(ic->conn);
- }
}
struct rds_ib_mr_pool *rds_ib_create_mr_pool(struct rds_ib_device *rds_ibdev)
diff --git a/net/rds/iw_rdma.c b/net/rds/iw_rdma.c
index de4a1b1..b25d785 100644
--- a/net/rds/iw_rdma.c
+++ b/net/rds/iw_rdma.c
@@ -245,11 +245,8 @@ void __rds_iw_destroy_conns(struct list_head *list, spinlock_t *list_lock)
INIT_LIST_HEAD(list);
spin_unlock_irq(list_lock);
- list_for_each_entry_safe(ic, _ic, &tmp_list, iw_node) {
- if (ic->conn->c_passive)
- rds_conn_destroy(ic->conn->c_passive);
+ list_for_each_entry_safe(ic, _ic, &tmp_list, iw_node)
rds_conn_destroy(ic->conn);
- }
}
static void rds_iw_set_scatterlist(struct rds_iw_scatterlist *sg,
--
1.6.3.3
^ permalink raw reply related
* Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock
From: Stephen Hemminger @ 2009-10-30 18:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <4AEB26BB.1050007@gmail.com>
On Fri, 30 Oct 2009 18:47:39 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Stephen Hemminger a écrit :
> > The ethtool operation to blink LED can take an indeterminately long time,
> > blocking out other operations (via rtnl_lock). This patch is an attempt
> > to work around the problem.
> >
> > It does need more discussion, because it will mean that drivers that formerly
> > were protected from changes during blink aren't. For example, user could
> > start device blinking, and then plug in cable causing change netlink event
> > to change state or pull cable and have device come down.
> >
> > The other possibility is to do this on a driver by driver basis
> > which is more effort.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> >
> > --- a/net/core/ethtool.c 2009-10-30 10:27:23.621917624 -0700
> > +++ b/net/core/ethtool.c 2009-10-30 10:35:53.787670774 -0700
> > @@ -17,6 +17,7 @@
> > #include <linux/errno.h>
> > #include <linux/ethtool.h>
> > #include <linux/netdevice.h>
> > +#include <linux/rtnetlink.h>
> > #include <asm/uaccess.h>
> >
> > /*
> > @@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
> > static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
> > {
> > struct ethtool_value id;
> > + int err;
> > + static int busy;
> >
> > if (!dev->ethtool_ops->phys_id)
> > return -EOPNOTSUPP;
> > @@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
> > if (copy_from_user(&id, useraddr, sizeof(id)))
> > return -EFAULT;
> >
> > - return dev->ethtool_ops->phys_id(dev, id.data);
> > + if (busy)
> > + return -EBUSY;
> > +
> > + /* This operation may take a long time, drop lock */
> > + busy = 1;
> > + dev_hold(dev);
> > + rtnl_unlock();
> > +
> > + err = dev->ethtool_ops->phys_id(dev, id.data);
> > +
> > + rtnl_lock();
> > + dev_put(dev);
> > + busy = 0;
> > +
> > + return err;
> > }
> >
>
> It seems reasonable, but why have a global 'busy' flag, and not
> private to each netdev ?
>
Because that is what old behaviour was (one blink at a time).
--
^ permalink raw reply
* Re: [PATCH] sky2: set carrier off in probe
From: David Miller @ 2009-10-30 18:20 UTC (permalink / raw)
To: herbert; +Cc: bphilips, shemminger, netdev
In-Reply-To: <20091030181632.GA9530@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 30 Oct 2009 14:16:32 -0400
> On Fri, Oct 30, 2009 at 11:10:27AM -0700, David Miller wrote:
> .
>> Because the driver must start in state with carrier off anyways,
>> so that we get the transition event when the device comes up
>> from link down to link up.
>
> Well we want all drivers to start in state NOCARRIER. However,
> a freshly allocated netdev has the NOCARRIER bit off. This means
> every single driver has to set the NOCARRIER bit.
>
> It would seem much easier to ensure that the NOCARRIER bit is set
> in a newly allocated netdev.
Since many drivers (especially virtual software ones) do not manage
carrier state and therefore that's why we start in state carrier on.
We've had this discussion a few times before, most recently with
Rusty wrt. virtio :-)
^ permalink raw reply
* Re: [PATCH] sky2: set carrier off in probe
From: Herbert Xu @ 2009-10-30 18:16 UTC (permalink / raw)
To: David Miller; +Cc: bphilips, shemminger, netdev
In-Reply-To: <20091030.111027.234994529.davem@davemloft.net>
On Fri, Oct 30, 2009 at 11:10:27AM -0700, David Miller wrote:
.
> Because the driver must start in state with carrier off anyways,
> so that we get the transition event when the device comes up
> from link down to link up.
Well we want all drivers to start in state NOCARRIER. However,
a freshly allocated netdev has the NOCARRIER bit off. This means
every single driver has to set the NOCARRIER bit.
It would seem much easier to ensure that the NOCARRIER bit is set
in a newly allocated netdev.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: William Allen Simpson @ 2009-10-30 18:11 UTC (permalink / raw)
To: Linux Kernel Network Developers
In-Reply-To: <4AEAC763.4070200@gmail.com>
William Allen Simpson wrote:
> First of all, this is my first attempt at locks, so I'd like early review.
>
> Secondly, scripts/checkpatch.pl tells me:
>
> ERROR: do not initialise statics to 0 or NULL
> #95: FILE: net/ipv4/tcp.c:2977:
> +static struct tcp_cookie_secret *tcp_secret_generating = NULL;
>
> They need to be NULL, and I'm not planning on exporting them, so what's
> the preferred mechanism?
>
> (I've grep'd many other instances of statics = 0 or NULL, so I'm not
> alone.)
>
I've found a void __init tcp_init() that seems to be used to initialize
some things, so I've stuck the NULL in there.
Still would like somebody to assure me that I've used the locks correctly.
^ permalink raw reply
* Re: [PATCH 2/3] net: TCP thin linear timeouts
From: William Allen Simpson @ 2009-10-30 18:11 UTC (permalink / raw)
To: Rick Jones
Cc: apetlund, Ilpo Järvinen, Arnd Hannemann, Eric Dumazet,
Netdev, LKML, shemminger, David Miller
In-Reply-To: <4AEB2362.5060601@hp.com>
Rick Jones wrote:
> apetlund@simula.no wrote:
>>> Just how thin can a thin stream be when a thin stream is found thin?
>>> (to the cadence of "How much wood could a woodchuck chuck if a
>>> woodchuck could chuck wood?")
>
>>> Does a stream get so thin that a user's send could not be split into
>>> four,
>>> sub-MSS TCP segments?
>>
>>
>> That was a nifty idea: Anti-Nagle the segments to be able to trigger fast
>> retransmissions. I think it is possible.
>>
>> Besides using more resources on each send, this scheme will introduce the
>> need to delay parts of the segment, which is undesirable for
>> time-dependent applications (the intended target of the mechanisms).
>>
>> I think it would be fun to implement and play around with such a
>> mechanism
>> to see the effects.
>
> Indeed, it does feel a bit "anti-nagle" but at the same time, these thin
> streams are supposed to be quite rare right? I mean we have survived 20
> odd years of congestion control and fast retransmission without it being
> a big issue.
>
> They are also supposed to not have terribly high bandwidth requirements
> yes? Suppose that instead of an explicit "I promise to be thin"
> setsockopt(), they instead set a Very Small (tm) in today's thinking
> socket buffer size and the stack then picks the MSS to be no more than
> 1/4 that size? Or for that matter, assuming the permissions are
> acceptable, the thin application makes a setsockopt(TCP_MAXSEG) call
> such that the actual MSS is small enough to allow the send()'s to be
> four (or more) segments. And, if one wants to spin-away the anti-Nagle,
> Nagle is defined by the send() being smaller than the MSS, so if the MSS
> is smaller, it isn't anti-Nagle :)
>
This is not a new idea. Folks used to set the MSS really low for M$
Windows, so that their short little packets went over dialup links more
quickly and they saw a little bit more of their graphic as it crawled to
the screen. Even though it was actually slower in total time, it "felt"
faster because of the continuing visual feedback. It depended upon VJ
Header Prediction to keep the overhead down for the link.
These are/were called "TCP mice", and the result was routers and servers
being nibbled by mice. Not pleasant.
> Further blue-skying...
>
> If SACK were also enabled, it would seem that only loss of the last
> segment in the "thin train" would be an issue? Presumably, the thin
> stream receiver would be in a position to detect this, perhaps with an
> application-level timeout. Whether then it would suffice to allow the
> receiving app to make a setsockopt() call to force an extra ACK or two
> I'm not sure. Perhaps if the thin-stream had a semi-aggressive
> "heartbeat" going...
>
Heartbeats are the usual solution for gaming. Handles a host of
issues, including detection of clients that have become unreachable.
(No, these are not the same as TCP keep-alives.)
Beside my code in the field and widespread discussion, I know that Paul
Francis had several related papers a decade or so ago. My memory is that
younger game coders weren't particularly avid readers....
> But it does seem that it should be possible to deal with this sort of
> thing without having to make wholesale changes to TCP's RTO policies and
> whatnot?
>
Yep.
^ permalink raw reply
* Re: [PATCH] sky2: set carrier off in probe
From: David Miller @ 2009-10-30 18:11 UTC (permalink / raw)
To: shemminger; +Cc: bphilips, netdev
In-Reply-To: <20091030083452.68ca227e@nehalam>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Fri, 30 Oct 2009 08:34:52 -0700
> Why not fix the problem in a generic way?
Drivers still need to make sure carrier is off when their
->open() routine runs, so that the transition event from
link down to link up occurs properly when the device is brought
up.
So mucking around with this carrier test will only hide the
bugs, not make things easier.
^ permalink raw reply
* Re: [PATCH] sky2: set carrier off in probe
From: David Miller @ 2009-10-30 18:10 UTC (permalink / raw)
To: herbert; +Cc: bphilips, shemminger, netdev
In-Reply-To: <20091030155420.GA8345@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 30 Oct 2009 11:54:20 -0400
> David Miller <davem@davemloft.net> wrote:
>>
>> I agree that we should report something consistent before interface
>> up and 'no' is probably the best.
>>
>> I remember fixing something similar in other drivers a few months
>> ago.
>
> Can't we do this in one spot rather than having every driver
> duplicate this?
It doesn't matter if we do.
Because the driver must start in state with carrier off anyways,
so that we get the transition event when the device comes up
from link down to link up.
So many things depend upon that link state transition event, that
we're not saving anything by just mucking with the carrier test.
^ permalink raw reply
* Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock
From: Eric Dumazet @ 2009-10-30 17:47 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
In-Reply-To: <20091030104201.6bb03b23@nehalam>
Stephen Hemminger a écrit :
> The ethtool operation to blink LED can take an indeterminately long time,
> blocking out other operations (via rtnl_lock). This patch is an attempt
> to work around the problem.
>
> It does need more discussion, because it will mean that drivers that formerly
> were protected from changes during blink aren't. For example, user could
> start device blinking, and then plug in cable causing change netlink event
> to change state or pull cable and have device come down.
>
> The other possibility is to do this on a driver by driver basis
> which is more effort.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
>
> --- a/net/core/ethtool.c 2009-10-30 10:27:23.621917624 -0700
> +++ b/net/core/ethtool.c 2009-10-30 10:35:53.787670774 -0700
> @@ -17,6 +17,7 @@
> #include <linux/errno.h>
> #include <linux/ethtool.h>
> #include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
> #include <asm/uaccess.h>
>
> /*
> @@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
> static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
> {
> struct ethtool_value id;
> + int err;
> + static int busy;
>
> if (!dev->ethtool_ops->phys_id)
> return -EOPNOTSUPP;
> @@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
> if (copy_from_user(&id, useraddr, sizeof(id)))
> return -EFAULT;
>
> - return dev->ethtool_ops->phys_id(dev, id.data);
> + if (busy)
> + return -EBUSY;
> +
> + /* This operation may take a long time, drop lock */
> + busy = 1;
> + dev_hold(dev);
> + rtnl_unlock();
> +
> + err = dev->ethtool_ops->phys_id(dev, id.data);
> +
> + rtnl_lock();
> + dev_put(dev);
> + busy = 0;
> +
> + return err;
> }
>
It seems reasonable, but why have a global 'busy' flag, and not
private to each netdev ?
^ permalink raw reply
* RFC: netdev: allow ethtool physical id to drop rtnl_lock
From: Stephen Hemminger @ 2009-10-30 17:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev
The ethtool operation to blink LED can take an indeterminately long time,
blocking out other operations (via rtnl_lock). This patch is an attempt
to work around the problem.
It does need more discussion, because it will mean that drivers that formerly
were protected from changes during blink aren't. For example, user could
start device blinking, and then plug in cable causing change netlink event
to change state or pull cable and have device come down.
The other possibility is to do this on a driver by driver basis
which is more effort.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/net/core/ethtool.c 2009-10-30 10:27:23.621917624 -0700
+++ b/net/core/ethtool.c 2009-10-30 10:35:53.787670774 -0700
@@ -17,6 +17,7 @@
#include <linux/errno.h>
#include <linux/ethtool.h>
#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
#include <asm/uaccess.h>
/*
@@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
{
struct ethtool_value id;
+ int err;
+ static int busy;
if (!dev->ethtool_ops->phys_id)
return -EOPNOTSUPP;
@@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
if (copy_from_user(&id, useraddr, sizeof(id)))
return -EFAULT;
- return dev->ethtool_ops->phys_id(dev, id.data);
+ if (busy)
+ return -EBUSY;
+
+ /* This operation may take a long time, drop lock */
+ busy = 1;
+ dev_hold(dev);
+ rtnl_unlock();
+
+ err = dev->ethtool_ops->phys_id(dev, id.data);
+
+ rtnl_lock();
+ dev_put(dev);
+ busy = 0;
+
+ return err;
}
static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
^ permalink raw reply
* Re: [PATCH 2/3] net: TCP thin linear timeouts
From: Rick Jones @ 2009-10-30 17:33 UTC (permalink / raw)
To: apetlund
Cc: Ilpo Järvinen, Arnd Hannemann, Eric Dumazet, Netdev, LKML,
shemminger, David Miller
In-Reply-To: <6980f83370cc081eb82dc2e0bd65bcf4.squirrel@webmail.uio.no>
apetlund@simula.no wrote:
>> Just how thin can a thin stream be when a thin stream is found thin? (to
>> the cadence of "How much wood could a woodchuck chuck if a woodchuck could
>> chuck wood?")
>> Does a stream get so thin that a user's send could not be split into four,
>> sub-MSS TCP segments?
>
>
> That was a nifty idea: Anti-Nagle the segments to be able to trigger fast
> retransmissions. I think it is possible.
>
> Besides using more resources on each send, this scheme will introduce the
> need to delay parts of the segment, which is undesirable for
> time-dependent applications (the intended target of the mechanisms).
>
> I think it would be fun to implement and play around with such a mechanism
> to see the effects.
Indeed, it does feel a bit "anti-nagle" but at the same time, these thin streams
are supposed to be quite rare right? I mean we have survived 20 odd years of
congestion control and fast retransmission without it being a big issue.
They are also supposed to not have terribly high bandwidth requirements yes?
Suppose that instead of an explicit "I promise to be thin" setsockopt(), they
instead set a Very Small (tm) in today's thinking socket buffer size and the
stack then picks the MSS to be no more than 1/4 that size? Or for that matter,
assuming the permissions are acceptable, the thin application makes a
setsockopt(TCP_MAXSEG) call such that the actual MSS is small enough to allow
the send()'s to be four (or more) segments. And, if one wants to spin-away the
anti-Nagle, Nagle is defined by the send() being smaller than the MSS, so if the
MSS is smaller, it isn't anti-Nagle :)
Further blue-skying...
If SACK were also enabled, it would seem that only loss of the last segment in
the "thin train" would be an issue? Presumably, the thin stream receiver would
be in a position to detect this, perhaps with an application-level timeout.
Whether then it would suffice to allow the receiving app to make a setsockopt()
call to force an extra ACK or two I'm not sure. Perhaps if the thin-stream had
a semi-aggressive "heartbeat" going...
But it does seem that it should be possible to deal with this sort of thing
without having to make wholesale changes to TCP's RTO policies and whatnot?
rick jones
^ permalink raw reply
* Re: [PATCH] udev: create empty regular files to represent net interfaces
From: Greg KH @ 2009-10-30 17:13 UTC (permalink / raw)
To: Matt Domsch
Cc: dann frazier, Narendra_K, kay.sievers, linux-hotplug, netdev,
Jordan_Hargrave, Charles_Rose, bhutchings
In-Reply-To: <20091030171003.GA7523@auslistsprd01.us.dell.com>
On Fri, Oct 30, 2009 at 12:10:03PM -0500, Matt Domsch wrote:
> ethN is fundamentally a nondeterministic namespace, and trying to
> enforce determinism on it is, from all my attempts, impossible. Hence
> the desire to change the namespace. But there can be many
> different naming policies one might want (including the
> nondeterministic ethN policy), and for all other types of devices this
> isn't a problem - we can have all the policies we want, in parallel.
> Only for network devices we can't.
So pick one in your installer and stick with it. Doesn't seem that
complicated to me...
greg k-h
^ permalink raw reply
* [PATCH net-next-2.6] net: Introduce dev_get_by_name_rcu()
From: Eric Dumazet @ 2009-10-30 17:11 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Some workloads hit dev_base_lock rwlock pretty hard.
We can use RCU lookups to avoid touching this rwlock
(and avoid touching netdevice refcount)
netdevices are already freed after a RCU grace period, so this patch
adds no penalty at device dismantle time.
However, it adds a synchronize_rcu() call in dev_change_name()
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/linux/netdevice.h | 1
net/core/dev.c | 49 +++++++++++++++++++++++++++++-------
2 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 193b637..86773e1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1115,6 +1115,7 @@ extern void __dev_remove_pack(struct packet_type *pt);
extern struct net_device *dev_get_by_flags(struct net *net, unsigned short flags,
unsigned short mask);
extern struct net_device *dev_get_by_name(struct net *net, const char *name);
+extern struct net_device *dev_get_by_name_rcu(struct net *net, const char *name);
extern struct net_device *__dev_get_by_name(struct net *net, const char *name);
extern int dev_alloc_name(struct net_device *dev, const char *name);
extern int dev_open(struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 94f42a1..f54d8b8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -213,7 +213,7 @@ static int list_netdevice(struct net_device *dev)
write_lock_bh(&dev_base_lock);
list_add_tail(&dev->dev_list, &net->dev_base_head);
- hlist_add_head(&dev->name_hlist, dev_name_hash(net, dev->name));
+ hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
hlist_add_head_rcu(&dev->index_hlist,
dev_index_hash(net, dev->ifindex));
write_unlock_bh(&dev_base_lock);
@@ -230,7 +230,7 @@ static void unlist_netdevice(struct net_device *dev)
/* Unlink dev from the device chain */
write_lock_bh(&dev_base_lock);
list_del(&dev->dev_list);
- hlist_del(&dev->name_hlist);
+ hlist_del_rcu(&dev->name_hlist);
hlist_del_rcu(&dev->index_hlist);
write_unlock_bh(&dev_base_lock);
}
@@ -599,6 +599,32 @@ struct net_device *__dev_get_by_name(struct net *net, const char *name)
EXPORT_SYMBOL(__dev_get_by_name);
/**
+ * dev_get_by_name_rcu - find a device by its name
+ * @net: the applicable net namespace
+ * @name: name to find
+ *
+ * Find an interface by name.
+ * If the name is found a pointer to the device is returned.
+ * If the name is not found then %NULL is returned.
+ * The reference counters are not incremented so the caller must be
+ * careful with locks. The caller must hold RCU lock.
+ */
+
+struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)
+{
+ struct hlist_node *p;
+ struct net_device *dev;
+ struct hlist_head *head = dev_name_hash(net, name);
+
+ hlist_for_each_entry_rcu(dev, p, head, name_hlist)
+ if (!strncmp(dev->name, name, IFNAMSIZ))
+ return dev;
+
+ return NULL;
+}
+EXPORT_SYMBOL(dev_get_by_name_rcu);
+
+/**
* dev_get_by_name - find a device by its name
* @net: the applicable net namespace
* @name: name to find
@@ -614,11 +640,11 @@ struct net_device *dev_get_by_name(struct net *net, const char *name)
{
struct net_device *dev;
- read_lock(&dev_base_lock);
- dev = __dev_get_by_name(net, name);
+ rcu_read_lock();
+ dev = dev_get_by_name_rcu(net, name);
if (dev)
dev_hold(dev);
- read_unlock(&dev_base_lock);
+ rcu_read_unlock();
return dev;
}
EXPORT_SYMBOL(dev_get_by_name);
@@ -960,7 +986,12 @@ rollback:
write_lock_bh(&dev_base_lock);
hlist_del(&dev->name_hlist);
- hlist_add_head(&dev->name_hlist, dev_name_hash(net, dev->name));
+ write_unlock_bh(&dev_base_lock);
+
+ synchronize_rcu();
+
+ write_lock_bh(&dev_base_lock);
+ hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
write_unlock_bh(&dev_base_lock);
ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
@@ -1062,9 +1093,9 @@ void dev_load(struct net *net, const char *name)
{
struct net_device *dev;
- read_lock(&dev_base_lock);
- dev = __dev_get_by_name(net, name);
- read_unlock(&dev_base_lock);
+ rcu_read_lock();
+ dev = dev_get_by_name_rcu(net, name);
+ rcu_read_unlock();
if (!dev && capable(CAP_NET_ADMIN))
request_module("%s", name);
^ permalink raw reply related
* Re: [PATCH] udev: create empty regular files to represent net interfaces
From: Matt Domsch @ 2009-10-30 17:10 UTC (permalink / raw)
To: dann frazier
Cc: Narendra_K, greg, kay.sievers, linux-hotplug, netdev,
Jordan_Hargrave, Charles_Rose, bhutchings
In-Reply-To: <20091030160845.GA4547@lackof.org>
On Fri, Oct 30, 2009 at 10:08:45AM -0600, dann frazier wrote:
> On Fri, Oct 30, 2009 at 08:43:44PM +0530, Narendra_K@Dell.com wrote:
> >
> > >> This way the kernel has only one name, and so does userspace, and
> > >> everyone is happy.
> > >
> > >There are two issues, which really seem distinct to me.
> > >
> > >Users expect eth0 to map to first-onboard-nic. That's an
> > >installer issue (since the BIOS can already export this info)
> > >and I agree that if we want to "fix" that, we should fix it there.
> > >
> >
> > I agree that installers have to be fixed in the sense that they can be
> > told to find the right interface. But, they expect determinism and
> > depend on "eth0 to map to first-onboard-nic". Installer is one of the
> > applications that is affected by this and needs user intervention, if it
> > is not told about the right interface. I discussed installer as it is so
> > much part of a user experience.
>
> Right, but couldn't the installer do the work of scanning the SMBIOS
> to figure out which nics are onboard, and reorder the 'eth*' names
> such that these are first? This state could then be written out as
> udev rules so that they persist across reboots.
No, there is a catch-22. To be sure you know the "proper" ethN name
to assign a device based on an ordering, you have to know about all
the devices. When udev runs, one device at a time, it can only see
the current device and all those that have come before it, but it can't know
when all the drivers for all the NICs have been loaded. And if you
hotplug a device in later, it should presumably just go at the end of
the list, but after a reboot, it'll most likely show up somewhere in
the middle of the list. SMBIOS is static from boottime, not hotplug
aware. If I add a 4-port NIC in slot 3 after boot, it becomes
ethN..N+3. After reboot, it may well show up at completely different
places, and even the N..N+3 ordering of individiual ports on the card
aren't guaranteed to be consistent.
ethN is fundamentally a nondeterministic namespace, and trying to
enforce determinism on it is, from all my attempts, impossible. Hence
the desire to change the namespace. But there can be many
different naming policies one might want (including the
nondeterministic ethN policy), and for all other types of devices this
isn't a problem - we can have all the policies we want, in parallel.
Only for network devices we can't.
Stephen, I hadn't seen the ifalias field you added. I can see that
being helpful to a user (some tool can write a more meaningful string
to it), but I can't see it being useful programmatically. It still
doesn't get me to "ifconfig the NIC in slot 3 port 2" or "ifconfig the
NIC I booted from".
Thanks,
Matt
--
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & www.dell.com/linux
^ permalink raw reply
* RE: How to use gretap with bridge?
From: Neulinger, Nathan @ 2009-10-30 17:08 UTC (permalink / raw)
To: Herbert Xu, David S. Miller; +Cc: shemminger, netdev
In-Reply-To: <20091030155148.GA8283@gondor.apana.org.au>
Confirmed, this fixes the problem for me and even now allows changing
the mac access by request.
Now I'm on to figuring out MTU issues. Thanks!
-- Nathan
------------------------------------------------------------
Nathan Neulinger nneul@mst.edu
Missouri S&T Information Technology (573) 612-1412
System Administrator - Principal KD0DMH
> -----Original Message-----
> From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
> Sent: Friday, October 30, 2009 10:52 AM
> To: Neulinger, Nathan; David S. Miller
> Cc: shemminger@vyatta.com; netdev@vger.kernel.org
> Subject: Re: How to use gretap with bridge?
>
> Neulinger, Nathan <nneul@mst.edu> wrote:
> >
> > The above change fixes it for me, but I'm no expert on this chunk of
> > code. (Perhaps it it shouldn't set dev_addr at all?)
>
> OK, it was a stupid mistake on my part. I added a netdev ops
> struct for tap but didn't actually use it! Please let us know
> whether this patch fixes the problem.
>
> gre: Fix dev_addr clobbering for gretap
>
> Nathan Neulinger noticed that gretap devices get their MAC address
> from the local IP address, which results in invalid MAC addresses
> half of the time.
>
> This is because gretap is still using the tunnel netdev ops rather
> than the correct tap netdev ops struct.
>
> This patch also fixes changelink to not clobber the MAC address
> for the gretap case.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Thanks,
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 41ada99..1433338 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1464,7 +1464,7 @@ static void ipgre_tap_setup(struct net_device
> *dev)
>
> ether_setup(dev);
>
> - dev->netdev_ops = &ipgre_netdev_ops;
> + dev->netdev_ops = &ipgre_tap_netdev_ops;
> dev->destructor = free_netdev;
>
> dev->iflink = 0;
> @@ -1525,25 +1525,29 @@ static int ipgre_changelink(struct net_device
> *dev, struct nlattr *tb[],
> if (t->dev != dev)
> return -EEXIST;
> } else {
> - unsigned nflags = 0;
> -
> t = nt;
>
> - if (ipv4_is_multicast(p.iph.daddr))
> - nflags = IFF_BROADCAST;
> - else if (p.iph.daddr)
> - nflags = IFF_POINTOPOINT;
> + if (dev->type != ARPHRD_ETHER) {
> + unsigned nflags = 0;
>
> - if ((dev->flags ^ nflags) &
> - (IFF_POINTOPOINT | IFF_BROADCAST))
> - return -EINVAL;
> + if (ipv4_is_multicast(p.iph.daddr))
> + nflags = IFF_BROADCAST;
> + else if (p.iph.daddr)
> + nflags = IFF_POINTOPOINT;
> +
> + if ((dev->flags ^ nflags) &
> + (IFF_POINTOPOINT | IFF_BROADCAST))
> + return -EINVAL;
> + }
>
> ipgre_tunnel_unlink(ign, t);
> t->parms.iph.saddr = p.iph.saddr;
> t->parms.iph.daddr = p.iph.daddr;
> t->parms.i_key = p.i_key;
> - memcpy(dev->dev_addr, &p.iph.saddr, 4);
> - memcpy(dev->broadcast, &p.iph.daddr, 4);
> + if (dev->type != ARPHRD_ETHER) {
> + memcpy(dev->dev_addr, &p.iph.saddr, 4);
> + memcpy(dev->broadcast, &p.iph.daddr, 4);
> + }
> ipgre_tunnel_link(ign, t);
> netdev_state_change(dev);
> }
^ permalink raw reply
* Re: [PATCH] udev: create empty regular files to represent netinterfaces
From: dann frazier @ 2009-10-30 17:05 UTC (permalink / raw)
To: Narendra_K
Cc: greg, Matt_Domsch, kay.sievers, linux-hotplug, netdev,
Jordan_Hargrave, Charles_Rose, bhutchings
In-Reply-To: <EDA0A4495861324DA2618B4C45DCB3EE589684@blrx3m08.blr.amer.dell.com>
On Fri, Oct 30, 2009 at 10:23:57PM +0530, Narendra_K@Dell.com wrote:
>
> >> >There are two issues, which really seem distinct to me.
> >> >
> >> >Users expect eth0 to map to first-onboard-nic. That's an installer
> >> >issue (since the BIOS can already export this info) and I
> >agree that
> >> >if we want to "fix" that, we should fix it there.
> >> >
> >>
> >> I agree that installers have to be fixed in the sense that
> >they can be
> >> told to find the right interface. But, they expect determinism and
> >> depend on "eth0 to map to first-onboard-nic". Installer is
> >one of the
> >> applications that is affected by this and needs user
> >intervention, if
> >> it is not told about the right interface. I discussed
> >installer as it
> >> is so much part of a user experience.
> >
> >Right, but couldn't the installer do the work of scanning the
> >SMBIOS to figure out which nics are onboard, and reorder the
> >'eth*' names such that these are first? This state could then
> >be written out as udev rules so that they persist across reboots.
> >
>
> I suppose, with udev loading modules, the rules generated at runtime
> could run into the problem of duplicate names, if names are reordered in
> the kernel namespace. (I.e the eth* namespace). Hence idea of an
> alternate namespace.
I don't see a risk of duplicate names - after all drivers are loaded,
the installer can take the names enumerated by the kernel, figure out
what it thinks a preferrable order is (i.e. by querying SMBIOS), then
change the kernel names/mac mapping appropriately. Where can a
duplicate name become an issue using this method?
--
dann frazier
^ permalink raw reply
* Re: Connection tracking and vlan
From: Herbert Xu @ 2009-10-30 16:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Adayadil Thomas, netdev, Patrick McHardy
In-Reply-To: <4AEB1212.6010905@gmail.com>
On Fri, Oct 30, 2009 at 05:19:30PM +0100, Eric Dumazet wrote:
>
> No, Abayadi needs firewall rules (or RPF), before entering conntrack.
rp_filter doesn't help because routing occurs after conntrack,
so IP fragments from the untrusted side may have already been
folded into a fragment from the trusted side.
Yes netfilter rules pre-conntrack would help but I bet there
are a lot folks out there who don't have them.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* RE: [PATCH] udev: create empty regular files to represent netinterfaces
From: Narendra_K @ 2009-10-30 16:53 UTC (permalink / raw)
To: dannf
Cc: greg, Matt_Domsch, kay.sievers, linux-hotplug, netdev,
Jordan_Hargrave, Charles_Rose, bhutchings
In-Reply-To: <20091030160845.GA4547@lackof.org>
>> >There are two issues, which really seem distinct to me.
>> >
>> >Users expect eth0 to map to first-onboard-nic. That's an installer
>> >issue (since the BIOS can already export this info) and I
>agree that
>> >if we want to "fix" that, we should fix it there.
>> >
>>
>> I agree that installers have to be fixed in the sense that
>they can be
>> told to find the right interface. But, they expect determinism and
>> depend on "eth0 to map to first-onboard-nic". Installer is
>one of the
>> applications that is affected by this and needs user
>intervention, if
>> it is not told about the right interface. I discussed
>installer as it
>> is so much part of a user experience.
>
>Right, but couldn't the installer do the work of scanning the
>SMBIOS to figure out which nics are onboard, and reorder the
>'eth*' names such that these are first? This state could then
>be written out as udev rules so that they persist across reboots.
>
I suppose, with udev loading modules, the rules generated at runtime
could run into the problem of duplicate names, if names are reordered in
the kernel namespace. (I.e the eth* namespace). Hence idea of an
alternate namespace.
With regards,
Narendra K
^ permalink raw reply
* Re: How to use gretap with bridge?
From: Stephen Hemminger @ 2009-10-30 16:39 UTC (permalink / raw)
To: Herbert Xu; +Cc: Neulinger, Nathan, David S. Miller, netdev
In-Reply-To: <20091030155148.GA8283@gondor.apana.org.au>
On Fri, 30 Oct 2009 11:51:48 -0400
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> Neulinger, Nathan <nneul@mst.edu> wrote:
> >
> > The above change fixes it for me, but I'm no expert on this chunk of
> > code. (Perhaps it it shouldn't set dev_addr at all?)
>
> OK, it was a stupid mistake on my part. I added a netdev ops
> struct for tap but didn't actually use it! Please let us know
> whether this patch fixes the problem.
>
> gre: Fix dev_addr clobbering for gretap
>
> Nathan Neulinger noticed that gretap devices get their MAC address
> from the local IP address, which results in invalid MAC addresses
> half of the time.
>
> This is because gretap is still using the tunnel netdev ops rather
> than the correct tap netdev ops struct.
>
> This patch also fixes changelink to not clobber the MAC address
> for the gretap case.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Thanks,
Acked-by: Stephen Hemminger <shemminger@vyatta.com>
--
^ permalink raw reply
* Re: [PATCH] udev: create empty regular files to represent net interfaces
From: Stephen Hemminger @ 2009-10-30 16:34 UTC (permalink / raw)
To: Bryan Kadzban
Cc: Hannes Reinecke, Kay Sievers, Matt Domsch, dann frazier,
linux-hotplug, Narendra_K, netdev, Jordan_Hargrave, Charles_Rose,
Ben Hutchings
In-Reply-To: <4AEB12CC.1070300@kadzban.is-a-geek.net>
On Fri, 30 Oct 2009 09:22:36 -0700
Bryan Kadzban <bryan@kadzban.is-a-geek.net> wrote:
> Hannes Reinecke wrote:
> > And to throw in some bit of useless information;
>
> Stirring the pot a bit myself with this message...
>
> > The one reason I didn't to this was that a network interface is _not_
> > a file, but rather an abstract type which is known only internally in
> > the kernel (ie the one exemption from the 'everything is a file' UNIX
> > rule).
>
> Why? Why not make it a file? I've heard rumors of other Unix-like
> systems that do exactly that, FWIW.
>
> (Yes, I'm joking. Well, maybe half-joking... It'd be nice, but I don't
> expect it to happen.)
>
> > When I were to design this, I would be implementing network interface
> > _aliases_, so that a network interface could be accessed either by
> > name or by alias. This mechanism can then be managed by udev, much
> > like we (ie SUSE) is using it nowadays to assign the network
> > interface numbers.
>
> The problem with that, if I understand what you're suggesting, is the
> value of IFNAMSIZ, and the fact that it can't be made any bigger. All
> your aliases have to be IFNAMSIZ characters or less. And that's too
> short to be able to embed the same level of information as we get for
> e.g. disks. It's *barely* long enough to fit "mac-" plus 12 hex digits
> (for the MAC address), but is completely incapable of holding a USB bus
> path, for instance.
>
> (Not that you'd want to use path persistence for USB devices. But it is
> possible that you'd want it for some other setup, at which point it
> becomes impossible to use the same rules for USB.)
Not a big fan of multiple names, it is the wrong solution to the human
question of "what is eth0 really?". Router o/s use description field
for that kind of information.
I added ifalias to provide place to put user visible description information.
It is in latest kernels via sysfs and iproute utilities. Also plan to add
support for it in net-snmp.
--
^ 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