* Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit
From: Jamal Hadi Salim @ 2016-10-23 19:52 UTC (permalink / raw)
To: Xin Long
Cc: netdev@vger.kernel.org, Marcelo Ricardo Leitner, Vlad Yasevich,
Daniel Borkmann, David Miller, linux-sctp@vger.kernel.org,
Michael Tuexen, Eric Dumazet, Brenda Butler, gabor
In-Reply-To: <CADvbK_ci-aaX6-mgXTgR0mJm9ww7+48DmXH+m5R+-0PyMDH16g@mail.gmail.com>
On 16-10-23 02:20 PM, Xin Long wrote:
> This patch doesn't ignore all the ENOMEN cases, only after msg is
> enqueued in out queue/send queue, in the lower layer, when alloc
> new skb and copy data from old skb, if it fails to alloc new skb, sctp
> will ignore this ENOMEM, as this msg will be taken care by retransmit
> mechanism, it's reasonable and also safe, user can't feel that.
>
Yes, that part i got.
> But for the cases before enqueue, like in sctp_sendmsg,
> sctp_datamsg_from_user may return ENOMEM, this err will return
> back to user, and can't be ignored.
>
The hard part is distinguishing between the above case and real
failure.
I am assuming in the case above user is _not_ required to send
again. But in the general case they are required to send again.
Correct?
> So I don't really think we should change something in manpage, what
> do you think ? maybe a little explanation there is also nice, :)
Yes, that would help. In particular it should be clear what user space
is expected to do. While this is about sctp - I am assuming equivalent
behavior for all callers of sendxxx() regardless of protocol.
cheers,
jamal
^ permalink raw reply
* Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit
From: Xin Long @ 2016-10-23 18:20 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev@vger.kernel.org, Marcelo Ricardo Leitner, Vlad Yasevich,
Daniel Borkmann, David Miller, linux-sctp@vger.kernel.org,
Michael Tuexen, Eric Dumazet, Brenda Butler, gabor
In-Reply-To: <daecd6b5-47cf-c5de-90f5-445e0038dfce@mojatatu.com>
> I think the specific use case this patch addresses
> seems to have bitten us in an older kernel sctp (3.11?).
> A send() on a loaded network box caused the skb to
> alloc in what appears to be this code path and fail (problem
> is intermittent, so not 100% sure). errno seen was ENOMEM.
> Unfortunately the manpage for sendxxx sucks.
> It says "no memory available".
> [We'll fix the manpage if there is an appropriate answer].
>
> Two questions:
> a) Seems like we can safely ignore ENOMEM in user space
> at least for this use case. i.e the kernel will retry and
> eventually send this message. Is there any other scenario
> where we have to worry about ENOMEM showing up in user space?
>
> b) What is the general view of what sendXXX reaction oughta
> be from user space in presence of ENOMEM?
>
This patch doesn't ignore all the ENOMEN cases, only after msg is
enqueued in out queue/send queue, in the lower layer, when alloc
new skb and copy data from old skb, if it fails to alloc new skb, sctp
will ignore this ENOMEM, as this msg will be taken care by retransmit
mechanism, it's reasonable and also safe, user can't feel that.
But for the cases before enqueue, like in sctp_sendmsg,
sctp_datamsg_from_user may return ENOMEM, this err will return
back to user, and can't be ignored.
So I don't really think we should change something in manpage, what
do you think ? maybe a little explanation there is also nice, :)
^ permalink raw reply
* [PATCH net] sctp: fix the panic caused by route update
From: Xin Long @ 2016-10-23 17:01 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Vlad Yasevich, daniel
Commit 7303a1475008 ("sctp: identify chunks that need to be fragmented
at IP level") made the chunk be fragmented at IP level in the next round
if it's size exceed PMTU.
But there still is another case, PMTU can be updated if transport's dst
expires and transport's pmtu_pending is set in sctp_packet_transmit. If
the new PMTU is less than the chunk, the same issue with that commit can
be triggered.
So we should drop this packet and let it retransmit in another round
where it would be fragmented at IP level.
This patch is to fix it by checking the chunk size after PMTU may be
updated and dropping this packet if it's size exceed PMTU.
Fixes: 90017accff61 ("sctp: Add GSO support")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/output.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/sctp/output.c b/net/sctp/output.c
index 2a5c189..6cb0df8 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -418,6 +418,7 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
__u8 has_data = 0;
int gso = 0;
int pktcount = 0;
+ int auth_len = 0;
struct dst_entry *dst;
unsigned char *auth = NULL; /* pointer to auth in skb data */
@@ -510,7 +511,12 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
list_for_each_entry(chunk, &packet->chunk_list, list) {
int padded = SCTP_PAD4(chunk->skb->len);
- if (pkt_size + padded > tp->pathmtu)
+ if (chunk == packet->auth)
+ auth_len = padded;
+ else if (auth_len + padded + packet->overhead >
+ tp->pathmtu)
+ goto nomem;
+ else if (pkt_size + padded > tp->pathmtu)
break;
pkt_size += padded;
}
--
2.1.0
^ permalink raw reply related
* Re: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: Stephen Hemminger @ 2016-10-23 16:38 UTC (permalink / raw)
To: Shrijeet Mukherjee; +Cc: mst, tom, netdev, shm, roopa, nikolay
In-Reply-To: <1477109243-29520-1-git-send-email-shrijeet@gmail.com>
Overall, I am glad to see XDP support more widely available. Minor stuff
in implementation.
>
> +/* this function is not called from the receive_buf path directly as
> + * we want to use the page model for rx merge buffer and big buffers
> + * and not use the fast path for driving skb's around
> + */
> +static inline u32 do_xdp_prog(struct virtnet_info *vi,
> + struct receive_queue *rq,
> + void *buf, int offset, int len)
> +{
Do not mark non-trivial static functions as inline. The compiler will
do it anyway if it thinks it is appropriate.
+static int virtnet_xdp_query(struct net_device *dev)
Use bool here.
@@ -366,13 +420,22 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
...
if (unlikely(!curr_skb))
goto err_skb;
+
while (--num_buf) {
int num_skb_frags;
@@ -1878,7 +2000,6 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtnet_change_mtu(dev, mtu))
__virtio_clear_bit(vdev, VIRTIO_NET_F_MTU);
}
-
if (vi->any_header_sg)
dev->needed_headroom = vi->hdr_len;
This parts of the patch is white space creep. I.e other edits you did
that stayed around.
Do you have any performance numbers? Does the receive into pages hurt
the non XDP performance?
^ permalink raw reply
* [PATCH net] doc: update docbook annotations for socket and skb
From: Stephen Hemminger @ 2016-10-23 16:28 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Stephen Hemminger <sthemmin@microsoft.com>
The skbuff and sock structure both had missing parameter annotation
values.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
include/linux/skbuff.h | 1 +
include/net/sock.h | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 601258f..32810f2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -936,6 +936,7 @@ struct sk_buff_fclones {
/**
* skb_fclone_busy - check if fclone is busy
+ * @sk: socket
* @skb: buffer
*
* Returns true if skb is a fast clone, and its clone is not freed.
diff --git a/include/net/sock.h b/include/net/sock.h
index ebf75db..73c6b00 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -252,6 +252,7 @@ struct sock_common {
* @sk_pacing_rate: Pacing rate (if supported by transport/packet scheduler)
* @sk_max_pacing_rate: Maximum pacing rate (%SO_MAX_PACING_RATE)
* @sk_sndbuf: size of send buffer in bytes
+ * @sk_padding: unused element for alignment
* @sk_no_check_tx: %SO_NO_CHECK setting, set checksum in TX packets
* @sk_no_check_rx: allow zero checksum in RX packets
* @sk_route_caps: route capabilities (e.g. %NETIF_F_TSO)
@@ -302,7 +303,8 @@ struct sock_common {
* @sk_backlog_rcv: callback to process the backlog
* @sk_destruct: called at sock freeing time, i.e. when all refcnt == 0
* @sk_reuseport_cb: reuseport group container
- */
+ * @sk_rcu: used during RCU grace period
+ */
struct sock {
/*
* Now struct inet_timewait_sock also uses sock_common, so please just
--
2.9.3
^ permalink raw reply related
* Re: [PATCH -next] net: dsa: mv88e6xxx: use setup_timer to simplify the code
From: Andrew Lunn @ 2016-10-23 15:54 UTC (permalink / raw)
To: Wei Yongjun; +Cc: Vivien Didelot, Florian Fainelli, Wei Yongjun, netdev
In-Reply-To: <1477146480-28003-1-git-send-email-weiyj.lk@gmail.com>
On Sat, Oct 22, 2016 at 02:28:00PM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Use setup_timer function instead of initializing timer with the function
> and data fields.
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/9] alx: refactor descriptor allocation
From: David Miller @ 2016-10-23 15:50 UTC (permalink / raw)
To: tobias.regnery; +Cc: jcliburn, chris.snook, netdev
In-Reply-To: <4118b3b705404f4ce27aaa389697c12f5319aaef.1477044918.git.tobias.regnery@gmail.com>
From: Tobias Regnery <tobias.regnery@gmail.com>
Date: Fri, 21 Oct 2016 12:49:44 +0200
> + txq->tpd = alx->descmem.virt + offset;
> + txq->tpd_dma = alx->descmem.dma + offset;
If all the crazy casting isn't necessary here...
> + rxq->rrd = (void *)((u8 *)alx->descmem.virt + offset);
> + rxq->rrd_dma = alx->descmem.dma + offset;
> + offset += sizeof(struct alx_rrd) * alx->rx_ringsz;
> +
> + rxq->rfd = (void *)((u8 *)alx->descmem.virt + offset);
> + rxq->rfd_dma = alx->descmem.dma + offset;
> + offset += sizeof(struct alx_rfd) * alx->rx_ringsz;
Then it certainly isn't necessary here either.
Void pointer arithmatic is very clearly defined as operating on byte
quantities, so the cast is not necessary for the arithmetic nor the
final pointer type.
^ permalink raw reply
* [patch net-next] devlink: Prevent port_type_set() callback when it's not needed
From: Jiri Pirko @ 2016-10-23 15:43 UTC (permalink / raw)
To: netdev; +Cc: davem, eladr
From: Elad Raz <eladr@mellanox.com>
When a port_type_set() is been called and the new port type set is the same
as the old one, just return success.
Signed-off-by: Elad Raz <eladr@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
net/core/devlink.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 1b50630..d2fd736 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -608,6 +608,8 @@ static int devlink_port_type_set(struct devlink *devlink,
if (devlink->ops && devlink->ops->port_type_set) {
if (port_type == DEVLINK_PORT_TYPE_NOTSET)
return -EINVAL;
+ if (port_type == devlink_port->type)
+ return 0;
err = devlink->ops->port_type_set(devlink_port, port_type);
if (err)
return err;
--
2.5.5
^ permalink raw reply related
* [PATCH net 1/1] net sched filters: fix notification of filter delete with proper handle
From: Jamal Hadi Salim @ 2016-10-23 15:35 UTC (permalink / raw)
To: davem; +Cc: netdev, xiyou.wangcong, daniel, eric.dumazet, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/cls_api.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ee29a3..2b2a797 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -345,7 +345,8 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
if (err == 0) {
struct tcf_proto *next = rtnl_dereference(tp->next);
- tfilter_notify(net, skb, n, tp, fh,
+ tfilter_notify(net, skb, n, tp,
+ t->tcm_handle,
RTM_DELTFILTER, false);
if (tcf_destroy(tp, false))
RCU_INIT_POINTER(*back, next);
--
1.9.1
^ permalink raw reply related
* Fwd: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit
From: Jamal Hadi Salim @ 2016-10-23 15:32 UTC (permalink / raw)
To: netdev@vger.kernel.org, Xin Long, Marcelo Ricardo Leitner
Cc: Vlad Yasevich, Daniel Borkmann, David Miller,
linux-sctp@vger.kernel.org, Michael Tuexen, Eric Dumazet,
Brenda Butler, gabor
In-Reply-To: <c15cc225-608d-5268-d179-6a997c3cad62@mojatatu.com>
Sorry - I didnt mean to remove the mailing lists.
Please reply to this email instead.
cheers,
jamal
-------- Forwarded Message --------
Subject: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not
return ENOMEM err back in sctp_packet_transmit
Date: Sun, 23 Oct 2016 11:03:36 -0400
From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Xin Long <lucien.xin@gmail.com>
CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, Vlad Yasevich
<vyasevich@gmail.com>, daniel@iogearbox.net, gabor@mojatatu.com, Brenda
Butler <bjb@mojatatu.com>, David Miller <davem@davemloft.net>,
linux-sctp@vger.kernel.org <linux-sctp@vger.kernel.org>, Michael Tuexen
<tuexen@fh-muenster.de>, Eric Dumazet <edumazet@google.com>
I think the specific use case this patch addresses
seems to have bitten us in an older kernel sctp (3.11?).
A send() on a loaded network box caused the skb to
alloc in what appears to be this code path and fail (problem
is intermittent, so not 100% sure). errno seen was ENOMEM.
Unfortunately the manpage for sendxxx sucks.
It says "no memory available".
[We'll fix the manpage if there is an appropriate answer].
Two questions:
a) Seems like we can safely ignore ENOMEM in user space
at least for this use case. i.e the kernel will retry and
eventually send this message. Is there any other scenario
where we have to worry about ENOMEM showing up in user space?
b) What is the general view of what sendXXX reaction oughta
be from user space in presence of ENOMEM?
cheers,
jamal
On 16-09-08 05:44 AM, Xin Long wrote:
> As David and Marcelo's suggestion, ENOMEM err shouldn't return back to
> user in transmit path. Instead, sctp's retransmit would take care of
> the chunks that fail to send because of ENOMEM.
>
> This patch is only to do some release job when alloc_skb fails, not to
> return ENOMEM back any more.
>
> Besides, it also cleans up sctp_packet_transmit's err path, and fixes
> some issues in err path:
>
> - It didn't free the head skb in nomem: path.
> - No need to check nskb in no_route: path.
> - It should goto err: path if alloc_skb fails for head.
> - Not all the NOMEMs should free nskb.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/output.c | 47 ++++++++++++++++++++++-------------------------
> 1 file changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 1934933..8f490ff 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -442,14 +442,14 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
> * time. Application may notice this error.
> */
> pr_err_once("Trying to GSO but underlying device doesn't support it.");
> - goto nomem;
> + goto err;
> }
> } else {
> pkt_size = packet->size;
> }
> head = alloc_skb(pkt_size + MAX_HEADER, gfp);
> if (!head)
> - goto nomem;
> + goto err;
> if (gso) {
> NAPI_GRO_CB(head)->last = head;
> skb_shinfo(head)->gso_type = sk->sk_gso_type;
> @@ -470,8 +470,12 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
> }
> }
> dst = dst_clone(tp->dst);
> - if (!dst)
> - goto no_route;
> + if (!dst) {
> + if (asoc)
> + IP_INC_STATS(sock_net(asoc->base.sk),
> + IPSTATS_MIB_OUTNOROUTES);
> + goto nodst;
> + }
> skb_dst_set(head, dst);
>
> /* Build the SCTP header. */
> @@ -622,8 +626,10 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
> if (!gso)
> break;
>
> - if (skb_gro_receive(&head, nskb))
> + if (skb_gro_receive(&head, nskb)) {
> + kfree_skb(nskb);
> goto nomem;
> + }
> nskb = NULL;
> if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs >=
> sk->sk_gso_max_segs))
> @@ -717,18 +723,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
> }
> head->ignore_df = packet->ipfragok;
> tp->af_specific->sctp_xmit(head, tp);
> + goto out;
>
> -out:
> - sctp_packet_reset(packet);
> - return err;
> -no_route:
> - kfree_skb(head);
> - if (nskb != head)
> - kfree_skb(nskb);
> -
> - if (asoc)
> - IP_INC_STATS(sock_net(asoc->base.sk), IPSTATS_MIB_OUTNOROUTES);
> +nomem:
> + if (packet->auth && list_empty(&packet->auth->list))
> + sctp_chunk_free(packet->auth);
>
> +nodst:
> /* FIXME: Returning the 'err' will effect all the associations
> * associated with a socket, although only one of the paths of the
> * association is unreachable.
> @@ -737,22 +738,18 @@ no_route:
> * required.
> */
> /* err = -EHOSTUNREACH; */
> -err:
> - /* Control chunks are unreliable so just drop them. DATA chunks
> - * will get resent or dropped later.
> - */
> + kfree_skb(head);
>
> +err:
> list_for_each_entry_safe(chunk, tmp, &packet->chunk_list, list) {
> list_del_init(&chunk->list);
> if (!sctp_chunk_is_data(chunk))
> sctp_chunk_free(chunk);
> }
> - goto out;
> -nomem:
> - if (packet->auth && list_empty(&packet->auth->list))
> - sctp_chunk_free(packet->auth);
> - err = -ENOMEM;
> - goto err;
> +
> +out:
> + sctp_packet_reset(packet);
> + return err;
> }
>
> /********************************************************************
>
^ permalink raw reply
* Re: [PATCH net] sched, cls: don't dump kernel addr into tc monitors on delete event
From: Jamal Hadi Salim @ 2016-10-23 15:24 UTC (permalink / raw)
To: Cong Wang
Cc: Daniel Borkmann, David Miller, Linux Kernel Network Developers,
Alexei Starovoitov
In-Reply-To: <CAM_iQpW4RdVo72=nQVaehkUCA98yq8+PyFQUTxis-Bx7XgVkFw@mail.gmail.com>
On 16-10-18 07:14 PM, Cong Wang wrote:
> On Tue, Oct 18, 2016 at 2:21 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> I was sitting on this patch I was going to send ;->
>> Does this resolve it?
>
> Your patch makes more sense to me. Maybe we can remove the
> event != RTM_DELTFILTER special case too?
>
We cant entirely get rid of that. Events other than
deletion require that a dump follows.
Probably what you mean is:
We could remove extra line inside "if (RTM_DELTFILTER != event)"
which does "tcm->tcm_handle = 0;"
It would be more reasonable to actually audit the callers of
tcf_fill_node() and make sure they pass the correct fh.
In any case, can we make that a followup - because this is a bug
fix?
I will send it and you could still comment if you dont find that
sufficient.
cheers,
jamal
^ permalink raw reply
* [PATCH net-next 2/2] firewire: net: set initial MTU = 1500 unconditionally, fix IPv6 on some CardBus cards
From: Stefan Richter @ 2016-10-23 14:30 UTC (permalink / raw)
To: David S. Miller; +Cc: Wilson, netdev, linux1394-devel, linux-kernel, Jarod
In-Reply-To: <20161023162903.4166a35d@kant>
firewire-net, like the older eth1394 driver, reduced the initial MTU to
less than 1500 octets if the local link layer controller's asynchronous
packet reception limit was lower.
This is bogus, since this reception limit does not have anything to do
with the transmission limit. Neither did this reduction affect the TX
path positively, nor could it prevent link fragmentation at the RX path.
Many FireWire CardBus cards have a max_rec of 9, causing an initial MTU
of 1024 - 16 = 1008. RFC 2734 and RFC 3146 allow a minimum max_rec = 8,
which would result in an initial MTU of 512 - 16 = 496. On such cards,
IPv6 could only be employed if the MTU was manually increased to 1280 or
more, i.e. IPv6 would not work without intervention from userland.
We now always initialize the MTU to 1500, which is the default according
to RFC 2734 and RFC 3146.
On a VIA VT6316 based CardBus card which was affected by this, changing
the MTU from 1008 to 1500 also increases TX bandwidth by 6 %.
RX remains unaffected.
CC: netdev@vger.kernel.org
CC: linux1394-devel@lists.sourceforge.net
CC: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
drivers/firewire/net.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 99379542b263..03715e7d9d92 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -1463,13 +1463,7 @@ static int fwnet_probe(struct fw_unit *unit,
goto out;
dev->local_fifo = dev->handler.offset;
- /*
- * Use the RFC 2734 default 1500 octets or the maximum payload
- * as initial MTU
- */
- net->mtu = min(1500U,
- (1U << (card->max_receive + 1))
- - RFC2374_FRAG_HDR_SIZE - IEEE1394_GASP_HDR_SIZE);
+ net->mtu = 1500U;
net->min_mtu = ETH_MIN_MTU;
net->max_mtu = ETH_MAX_MTU;
--
Stefan Richter
-======----- =-=- =-===
http://arcgraph.de/sr/
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net-next 1/2] firewire: net: fix maximum possible MTU
From: Stefan Richter @ 2016-10-23 14:29 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux1394-devel, Jarod Wilson, linux-kernel
In-Reply-To: <20161023011824.GE32569@redhat.com>
Commit b3e3893e1253 ("net: use core MTU range checking in misc drivers")
mistakenly introduced an upper limit for firewire-net's MTU based on the
local link layer controller's reception capability. Revert this. Neither
RFC 2734 nor our implementation impose any particular upper limit.
Actually, to be on the safe side and to make the code explicit, set
ETH_MAX_MTU = 65535 as upper limit now.
(I replaced sizeof(struct rfc2734_header) by the equivalent
RFC2374_FRAG_HDR_SIZE in order to avoid distracting long/int conversions.)
Fixes: b3e3893e1253('net: use core MTU range checking in misc drivers')
CC: netdev@vger.kernel.org
CC: linux1394-devel@lists.sourceforge.net
CC: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
drivers/firewire/net.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 8430222151fc..99379542b263 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -1467,10 +1467,11 @@ static int fwnet_probe(struct fw_unit *unit,
* Use the RFC 2734 default 1500 octets or the maximum payload
* as initial MTU
*/
- net->max_mtu = (1 << (card->max_receive + 1))
- - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
- net->mtu = min(1500U, net->max_mtu);
+ net->mtu = min(1500U,
+ (1U << (card->max_receive + 1))
+ - RFC2374_FRAG_HDR_SIZE - IEEE1394_GASP_HDR_SIZE);
net->min_mtu = ETH_MIN_MTU;
+ net->max_mtu = ETH_MAX_MTU;
/* Set our hardware address while we're at it */
ha = (union fwnet_hwaddr *)net->dev_addr;
--
Stefan Richter
-======----- =-=- =-===
http://arcgraph.de/sr/
^ permalink raw reply related
* Re: [RFC 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Rangankar, Manish @ 2016-10-23 14:04 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: lduncan@suse.com, cleech@redhat.com, martin.petersen@oracle.com,
jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <20161019100253.vxqxp5fhoxrlt6ay@linux-x5ow.site>
On 19/10/16 3:32 PM, "Johannes Thumshirn" <jthumshirn@suse.de> wrote:
>On Wed, Oct 19, 2016 at 01:01:10AM -0400, manish.rangankar@cavium.com
>wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
>> for 41000 Series Converged Network Adapters by QLogic.
>>
>> This patch consists of following changes:
>> - MAINTAINERS Makefile and Kconfig changes for qedi,
>> - PCI driver registration,
>> - iSCSI host level initialization,
>> - Debugfs and log level infrastructure.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>
>[...]
>
>> +/* MSI-X fastpath handler code */
>> +static irqreturn_t qedi_msix_handler(int irq, void *dev_id)
>> +{
>> + struct qedi_fastpath *fp = dev_id;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + bool wake_io_thread = true;
>> +
>> + qed_sb_ack(fp->sb_info, IGU_INT_DISABLE, 0);
>> +
>> +process_again:
>> + wake_io_thread = qedi_process_completions(fp);
>> + if (wake_io_thread) {
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_DISC,
>> + "process already running\n");
>> + }
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_update_sb_idx(fp->sb_info);
>> +
>> + /* Check for more work */
>> + rmb();
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_ack(fp->sb_info, IGU_INT_ENABLE, 1);
>> + else
>> + goto process_again;
>> +
>> + return IRQ_HANDLED;
>> +}
>
>You might want to consider workqueues here.
If there is no serious objection with current per-cpu threads
implementation
then we will like to do workqueue changes just after first submission.
This is because,
for this change we have go through complete validation cycle on our part.
Thanks,
Manish R.
^ permalink raw reply
* Re: [PATCH v4 01/10] ethernet: add sun8i-emac driver
From: Rami Rosen @ 2016-10-23 12:08 UTC (permalink / raw)
To: Corentin Labbe
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, David Miller, Florian Fainelli,
andrew-g2DYL2Zd6BY, Netdev, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1475828757-926-2-git-send-email-clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Corentin,
Trivial comment: rx_saf_fai and rx_daf_fail are not used. This implies that also
rx_saf_error and rx_daf_error should be removed:
> +static const char const estats_str[][ETH_GSTRING_LEN] = {
...
> + "rx_header_error",
> + "rx_overflow_error",
> + "rx_saf_error",
> + "rx_daf_error",
> + "rx_buf_error",
...
> +
> +struct sun8i_emac_stats {
> + u64 rx_payload_error;
...
> + u64 rx_overflow_error;
> + u64 rx_saf_fail;
> + u64 rx_daf_fail;
> + u64 tx_used_desc;
> + u64 napi_schedule;
> + u64 napi_underflow;
> +};
> +
Trivial: typo, should be: can transfer
> +/* The datasheet said that each descriptor can transfers up to 4096bytes
> + * But latter, a register documentation reduce that value to 2048
Regards,
Rami Rosen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 01/10] ethernet: add sun8i-emac driver
From: LABBE Corentin @ 2016-10-23 8:55 UTC (permalink / raw)
To: Joe Perches
Cc: robh+dt, mark.rutland, maxime.ripard, wens, linux, davem,
f.fainelli, andrew, netdev, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <1475852559.1945.2.camel@perches.com>
On Fri, Oct 07, 2016 at 08:02:39AM -0700, Joe Perches wrote:
> On Fri, 2016-10-07 at 10:25 +0200, Corentin Labbe wrote:
> > This patch add support for sun8i-emac ethernet MAC hardware.
> > It could be found in Allwinner H3/A83T/A64 SoCs.
>
> trivial notes:
>
> > diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c b/drivers/net/ethernet/allwinner/sun8i-emac.c
> []
> > +static const char const estats_str[][ETH_GSTRING_LEN] = {
>
> one too many const
>
> > +/* MAGIC value for knowing if a descriptor is available or not */
> > +#define DCLEAN cpu_to_le32(BIT(16) | BIT(14) | BIT(12) | BIT(10) | BIT(9))
>
> Aren't there #defines for these bits?
>
> > +static void sun8i_emac_flow_ctrl(struct sun8i_emac_priv *priv, int duplex,
> > + int fc)
> > +{
> > + u32 flow = 0;
> > +
> > + flow = readl(priv->base + EMAC_RX_CTL0);
> > + if (fc & EMAC_FLOW_RX)
> > + flow |= BIT(16);
> > + else
> > + flow &= ~BIT(16);
> > + writel(flow, priv->base + EMAC_RX_CTL0);
> > +
> > + flow = readl(priv->base + EMAC_TX_FLOW_CTL);
> > + if (fc & EMAC_FLOW_TX)
> > + flow |= BIT(0);
> > + else
> > + flow &= ~BIT(0);
>
> more magic bits that could be #defines
>
> > +static int sun8i_emac_rx_from_ddesc(struct net_device *ndev, int i)
> > +{
> > []
> > + /* the checksum or length of received frame's payload is wrong*/
> > + if (dstatus & BIT(0)) {
> []
> > + if (dstatus & BIT(1)) {
> []
> > + if ((dstatus & BIT(3))) {
>
> etc...
Thanks for the review, I will fix all thoses issue.
Regards
Corentin Labbe
^ permalink raw reply
* Re: [PATCH] batman-adv: Revert "use core MTU range checking in misc drivers"
From: Sven Eckelmann @ 2016-10-23 7:17 UTC (permalink / raw)
To: Jarod Wilson
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
b.a.t.m.a.n-ZwoEplunGu2X36UT3dwllkB+6BGkLq7r,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161023010826.GD32569-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 701 bytes --]
On Samstag, 22. Oktober 2016 21:08:26 CEST Jarod Wilson wrote:
[...]
> You're going to
> need more than just this revert though, since batman-adv calls
> ether_setup, which will set min_mtu = 68, max_mtu = 1500, unless
> batadv_hardif_min_mtu() always returns something 1500 or less.
It does only returns 1500 or less at the moment.
return min_t(int, min_mtu - batadv_max_header_len(), ETH_DATA_LEN);
> Actually,
> looking at that, you could omit the mtu < 68 bit from
> batadv_interface_change_mtu() too, since that'll already get done in the
> core, but I have no clue what you need for max_mtu.
I would like to get this revert through net-next.git before anything else.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v7] net: ip, diag -- Add diag interface for raw sockets
From: Cyrill Gorcunov @ 2016-10-23 7:12 UTC (permalink / raw)
To: David Miller
Cc: netdev, dsa, eric.dumazet, jhs, linux-kernel, kuznet, jmorris,
yoshfuji, kaber, avagin, stephen
In-Reply-To: <20161022.185527.935058828725905432.davem@davemloft.net>
On Sat, Oct 22, 2016 at 06:55:27PM -0400, David Miller wrote:
> From: Cyrill Gorcunov <gorcunov@gmail.com>
> Date: Fri, 21 Oct 2016 13:03:44 +0300
>
> > In criu we are actively using diag interface to collect sockets
> > present in the system when dumping applications. And while for
> > unix, tcp, udp[lite], packet, netlink it works as expected,
> > the raw sockets do not have. Thus add it.
>
> Applied, thanks.
Thanks to all for extensive comments and review!
^ permalink raw reply
* Re: [PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers
From: Jarod Wilson @ 2016-10-23 1:18 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-kernel, netdev, linux1394-devel
In-Reply-To: <20161022212759.228c7642@kant>
On Sat, Oct 22, 2016 at 09:27:59PM +0200, Stefan Richter wrote:
> Adding Cc: linux1394-devel, dropping several Ccs, no additional comment.
>
> On Oct 22 Stefan Richter wrote:
> > On Oct 20 Jarod Wilson wrote:
> > > firewire-net:
> > > - set min/max_mtu
> > > - remove fwnet_change_mtu
> > [...]
> > > --- a/drivers/firewire/net.c
> > > +++ b/drivers/firewire/net.c
> > > @@ -1349,15 +1349,6 @@ static netdev_tx_t fwnet_tx(struct sk_buff
> > > *skb, struct net_device *net) return NETDEV_TX_OK;
> > > }
> > >
> > > -static int fwnet_change_mtu(struct net_device *net, int new_mtu)
> > > -{
> > > - if (new_mtu < 68)
> > > - return -EINVAL;
> > > -
> > > - net->mtu = new_mtu;
> > > - return 0;
> > > -}
> > > -
> > > static const struct ethtool_ops fwnet_ethtool_ops = {
> > > .get_link = ethtool_op_get_link,
> > > };
> > > @@ -1366,7 +1357,6 @@ static const struct net_device_ops
> > > fwnet_netdev_ops = { .ndo_open = fwnet_open,
> > > .ndo_stop = fwnet_stop,
> > > .ndo_start_xmit = fwnet_tx,
> > > - .ndo_change_mtu = fwnet_change_mtu,
> > > };
> > >
> > > static void fwnet_init_dev(struct net_device *net)
> > > @@ -1435,7 +1425,6 @@ static int fwnet_probe(struct fw_unit *unit,
> > > struct net_device *net;
> > > bool allocated_netdev = false;
> > > struct fwnet_device *dev;
> > > - unsigned max_mtu;
> > > int ret;
> > > union fwnet_hwaddr *ha;
> > >
> > > @@ -1478,9 +1467,10 @@ static int fwnet_probe(struct fw_unit *unit,
> > > * Use the RFC 2734 default 1500 octets or the maximum payload
> > > * as initial MTU
> > > */
> > > - max_mtu = (1 << (card->max_receive + 1))
> > > - - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> > > - net->mtu = min(1500U, max_mtu);
> > > + net->max_mtu = (1 << (card->max_receive + 1))
> > > + - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> > > + net->mtu = min(1500U, net->max_mtu);
> > > + net->min_mtu = ETH_MIN_MTU;
> > >
> > > /* Set our hardware address while we're at it */
> > > ha = (union fwnet_hwaddr *)net->dev_addr;
> >
> > Please preserve the current behavior, i.e. do not enforce any particular
> > upper bound. (Especially none based on the local link layer controller's
> > max_receive parameter.)
> >
> > BTW, after having read RFC 2734, RFC 3146, and the code, I am convinced
> > that net->mtu should be initialized to 1500, not less. But such a change
> > should be done in a separate patch.
Okay, since it's already merged in net-next, I can do a follow-up patch
here to set max_mtu to ETH_MAX_MTU (65535), which is the largest possible
size the kernel can handle, so far as I can tell. But as long as I'm going
to be in here, if we just want to use an initial mtu of 1500, I could
clean that up at the same time, and entirely remove the max_mtu
calculation stuff, if that's what you think is more correct here.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply
* Re: [PATCH] batman-adv: Revert "use core MTU range checking in misc drivers"
From: Jarod Wilson @ 2016-10-23 1:08 UTC (permalink / raw)
To: Sven Eckelmann; +Cc: netdev, davem, b.a.t.m.a.n, linux-kernel
In-Reply-To: <20161022074624.30033-1-sven@narfation.org>
On Sat, Oct 22, 2016 at 09:46:24AM +0200, Sven Eckelmann wrote:
> The maximum MTU is defined via the slave devices of an batman-adv
> interface. Thus it is not possible to calculate the max_mtu during the
> creation of the batman-adv device when no slave devices are attached. Doing
> so would for example break non-fragmentation setups which then
> (incorrectly) allow an MTU of 1500 even when underlying device cannot
> transport 1500 bytes + batman-adv headers.
>
> Checking the dynamically calculated max_mtu via the minimum of the slave
> devices MTU during .ndo_change_mtu is also used by the bridge interface.
>
> Cc: Jarod Wilson <jarod@redhat.com>
> Fixes: b3e3893e1253 ("net: use core MTU range checking in misc drivers")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Original patch + my comment: https://patchwork.ozlabs.org/patch/684722/
>
> I just got informed about this patch when it was already applied in net-next.
> So I can only ask for an revet of the batman-adv parts
Apologies, I tried to cc everyone I could find in MAINTAINERS, but your
name wasn't one of the three listed for batman devices. You're going to
need more than just this revert though, since batman-adv calls
ether_setup, which will set min_mtu = 68, max_mtu = 1500, unless
batadv_hardif_min_mtu() always returns something 1500 or less. Actually,
looking at that, you could omit the mtu < 68 bit from
batadv_interface_change_mtu() too, since that'll already get done in the
core, but I have no clue what you need for max_mtu.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply
* Re: [PATCH v7] net: ip, diag -- Add diag interface for raw sockets
From: David Miller @ 2016-10-22 22:55 UTC (permalink / raw)
To: gorcunov
Cc: netdev, dsa, eric.dumazet, jhs, linux-kernel, kuznet, jmorris,
yoshfuji, kaber, avagin, stephen
In-Reply-To: <20161021100344.GM1847@uranus.lan>
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Fri, 21 Oct 2016 13:03:44 +0300
> In criu we are actively using diag interface to collect sockets
> present in the system when dumping applications. And while for
> unix, tcp, udp[lite], packet, netlink it works as expected,
> the raw sockets do not have. Thus add it.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] netfilter: don't permit unprivileged writes to global state via sysctls
From: David Miller @ 2016-10-22 22:43 UTC (permalink / raw)
To: jann; +Cc: pablo, kuznet, jmorris, yoshfuji, netdev, netfilter-devel
In-Reply-To: <20161022212342.GD3334@pc.thejh.net>
From: Jann Horn <jann@thejh.net>
Date: Sat, 22 Oct 2016 23:23:42 +0200
> On Thu, Oct 20, 2016 at 02:37:47PM -0400, David Miller wrote:
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Thu, 20 Oct 2016 20:22:24 +0200
>>
>> > On Sat, Sep 24, 2016 at 12:21:04AM +0200, Jann Horn wrote:
>> >> This prevents the modification of nf_conntrack_max in unprivileged network
>> >> namespaces. For unprivileged network namespaces, ip_conntrack_max is kept
>> >> as a readonly sysctl in order to minimize potential compatibility issues.
>> >>
>> >> This patch should apply cleanly to the net tree.
>> >
>> > For the record: This patch looks good to me, but this legacy
>> > ip_conntrack sysctl code is now gone.
>> >
>> > I don't know what is the procedure to get this to -stable branches now
>> > that this cannot be pushed upstream.
>>
>> In the commit message for the -stable submission simply say "Not
>> applicable" in the upstream commit reference. Like:
>>
>> [ Upstream commit: Not applicable ]
>>
>> or something like that.
>
> Who should do that? Me, after getting a maintainer ack? Or the maintainer?
When the maintainer submits a patch to -stable, that's what they
add to the commit message.
^ permalink raw reply
* Re: [PATCH] netfilter: don't permit unprivileged writes to global state via sysctls
From: Jann Horn @ 2016-10-22 21:23 UTC (permalink / raw)
To: David Miller; +Cc: pablo, kuznet, jmorris, yoshfuji, netdev, netfilter-devel
In-Reply-To: <20161020.143747.1033652491220298518.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]
On Thu, Oct 20, 2016 at 02:37:47PM -0400, David Miller wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Thu, 20 Oct 2016 20:22:24 +0200
>
> > On Sat, Sep 24, 2016 at 12:21:04AM +0200, Jann Horn wrote:
> >> This prevents the modification of nf_conntrack_max in unprivileged network
> >> namespaces. For unprivileged network namespaces, ip_conntrack_max is kept
> >> as a readonly sysctl in order to minimize potential compatibility issues.
> >>
> >> This patch should apply cleanly to the net tree.
> >
> > For the record: This patch looks good to me, but this legacy
> > ip_conntrack sysctl code is now gone.
> >
> > I don't know what is the procedure to get this to -stable branches now
> > that this cannot be pushed upstream.
>
> In the commit message for the -stable submission simply say "Not
> applicable" in the upstream commit reference. Like:
>
> [ Upstream commit: Not applicable ]
>
> or something like that.
Who should do that? Me, after getting a maintainer ack? Or the maintainer?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net 0/7] qed*: fix series.
From: David Miller @ 2016-10-22 21:18 UTC (permalink / raw)
To: sudarsana.kalluru; +Cc: netdev, Ariel.Elior, Yuval.Mintz, manish.chopra
In-Reply-To: <1477039425-31341-1-git-send-email-sudarsana.kalluru@cavium.com>
From: <sudarsana.kalluru@cavium.com>
Date: Fri, 21 Oct 2016 04:43:38 -0400
> The patch series contains several minor bug fixes for qed/qede modules.
> Please consider applying this to 'net' branch.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] net: skip genenerating uevents for network namespaces that are exiting
From: David Miller @ 2016-10-22 21:00 UTC (permalink / raw)
To: avagin; +Cc: ebiederm, containers, linux-kernel, netdev
In-Reply-To: <1477017986-18889-1-git-send-email-avagin@openvz.org>
From: Andrei Vagin <avagin@openvz.org>
Date: Thu, 20 Oct 2016 19:46:26 -0700
> @@ -1525,6 +1530,9 @@ void netdev_unregister_kobject(struct net_device *ndev)
> {
> struct device *dev = &(ndev->dev);
>
> + if (!list_empty(&dev_net(ndev)->exit_list))
> + dev->kobj.uevent_suppress = 1;
> +
Please use "dev_set_uevent_suppress(dev, 1);"
^ 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