* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Ivan Khoronzhuk @ 2019-07-02 14:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190702164648.56ff0761@carbon>
On Tue, Jul 02, 2019 at 04:46:48PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 13:27:07 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Mon, Jul 01, 2019 at 01:40:59PM +0200, Jesper Dangaard Brouer wrote:
>> >
>> >I'm very skeptical about this approach.
>> >
>> >On Sun, 30 Jun 2019 20:23:43 +0300
>> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >
>> >> XDP rxqs can be same for ndevs running under same rx napi softirq.
>> >> But there is no ability to register same allocator for both rxqs,
>> >> by fact it can same rxq but has different ndev as a reference.
>> >
>> >This description is not very clear. It can easily be misunderstood.
>> >
>> >It is an absolute requirement that each RX-queue have their own
>> >page_pool object/allocator. (This where the performance comes from) as
>> >the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
>> >
>> >Your driver/hardware seems to have special case, where a single
>> >RX-queue can receive packets for two different net_device'es.
>> >
>> >Do you violate this XDP devmap redirect assumption[1]?
>> >[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
>> Seems that yes, but that's used only for trace for now.
>> As it runs under napi and flush clear dev_rx i must do it right in the
>> rx_handler. So next patchset version will have one patch less.
>>
>> Thanks!
>>
>> >
>> >
>> >> Due to last changes allocator destroy can be defered till the moment
>> >> all packets are recycled by destination interface, afterwards it's
>> >> freed. In order to schedule allocator destroy only after all users are
>> >> unregistered, add refcnt to allocator object and schedule to destroy
>> >> only it reaches 0.
>> >
>> >The guiding principles when designing an API, is to make it easy to
>> >use, but also make it hard to misuse.
>> >
>> >Your API change makes it easy to misuse the API. As it make it easy to
>> >(re)use the allocator pointer (likely page_pool) for multiple
>> >xdp_rxq_info structs. It is only valid for your use-case, because you
>> >have hardware where a single RX-queue delivers to two different
>> >net_devices. For other normal use-cases, this will be a violation.
>> >
>> >If I was a user of this API, and saw your xdp_allocator_get(), then I
>> >would assume that this was the normal case. As minimum, we need to add
>> >a comment in the code, about this specific/intended use-case. I
>> >through about detecting the misuse, by adding a queue_index to
>> >xdp_mem_allocator, that can be checked against, when calling
>> >xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
>> >the obvious mistake where queue_index mismatch).
>>
>> I can add, but not sure if it has or can have some conflicts with another
>> memory allocators, now or in future. Main here to not became a cornerstone
>> in some not obvious use-cases.
>>
>> So, for now, let it be in this way:
>>
>> --- a/include/net/xdp_priv.h
>> +++ b/include/net/xdp_priv.h
>> @@ -19,6 +19,7 @@ struct xdp_mem_allocator {
>> struct delayed_work defer_wq;
>> unsigned long defer_warn;
>> unsigned long refcnt;
>> + u32 queue_index;
>> };
>>
>> #endif /* __LINUX_NET_XDP_PRIV_H__ */
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index a44621190fdc..c4bf29810f4d 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -324,7 +324,7 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
>> return true;
>> }
>>
>> -static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
>> {
>> struct xdp_mem_allocator *xae, *xa = NULL;
>> struct rhashtable_iter iter;
>> @@ -336,7 +336,6 @@ static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>>
>> while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
>> if (xae->allocator == allocator) {
>> - xae->refcnt++;
>> xa = xae;
>> break;
>> }
>> @@ -386,9 +385,13 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> }
>> }
>>
>> - xdp_alloc = xdp_allocator_get(allocator);
>> + xdp_alloc = xdp_allocator_find(allocator);
>> if (xdp_alloc) {
>> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
>> + return -EINVAL;
>> +
>> xdp_rxq->mem.id = xdp_alloc->mem.id;
>> + xdp_alloc->refcnt++;
>
>This is now adjusted outside lock, not good.
In final it serves:
From f43a0b85838f75814cc93e5a724c4c7e5615f936 Mon Sep 17 00:00:00 2001
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Fri, 28 Jun 2019 03:17:24 +0300
Subject: [PATCH] xdp: allow same allocator usage
XDP rxqs can be same for ndevs running under same rx napi softirq.
But there is no ability to register same allocator for both rxqs,
by fact it can same rxq but has different ndev as a reference.
Due to last changes allocator destroy can be defered till the moment
all packets are recycled by destination interface, afterwards it's
freed. In order to schedule allocator destroy only after all users are
unregistered, add refcnt to allocator object and schedule to destroy
only it reaches 0.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
include/net/xdp_priv.h | 2 ++
net/core/xdp.c | 52 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
index 6a8cba6ea79a..9858a4057842 100644
--- a/include/net/xdp_priv.h
+++ b/include/net/xdp_priv.h
@@ -18,6 +18,8 @@ struct xdp_mem_allocator {
struct rcu_head rcu;
struct delayed_work defer_wq;
unsigned long defer_warn;
+ unsigned long refcnt;
+ u32 queue_index;
};
#endif /* __LINUX_NET_XDP_PRIV_H__ */
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 829377cc83db..090f26e4f793 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -98,6 +98,18 @@ static bool __mem_id_disconnect(int id, bool force)
WARN(1, "Request remove non-existing id(%d), driver bug?", id);
return true;
}
+
+ /* to avoid calling hash lookup twice, decrement refcnt here till it
+ * reaches zero, then it can be called from workqueue afterwards.
+ */
+ if (xa->refcnt)
+ xa->refcnt--;
+
+ if (xa->refcnt) {
+ mutex_unlock(&mem_id_lock);
+ return true;
+ }
+
xa->disconnect_cnt++;
/* Detects in-flight packet-pages for page_pool */
@@ -312,6 +324,30 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
return true;
}
+static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
+{
+ struct xdp_mem_allocator *xae, *xa = NULL;
+ struct rhashtable_iter iter;
+
+ rhashtable_walk_enter(mem_id_ht, &iter);
+ do {
+ rhashtable_walk_start(&iter);
+
+ while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
+ if (xae->allocator == allocator) {
+ xa = xae;
+ break;
+ }
+ }
+
+ rhashtable_walk_stop(&iter);
+
+ } while (xae == ERR_PTR(-EAGAIN));
+ rhashtable_walk_exit(&iter);
+
+ return xa;
+}
+
int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
enum xdp_mem_type type, void *allocator)
{
@@ -347,6 +383,20 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
}
}
+ mutex_lock(&mem_id_lock);
+ xdp_alloc = xdp_allocator_find(allocator);
+ if (xdp_alloc) {
+ /* One allocator per queue is supposed only */
+ if (xdp_alloc->queue_index != xdp_rxq->queue_index)
+ return -EINVAL;
+
+ xdp_rxq->mem.id = xdp_alloc->mem.id;
+ xdp_alloc->refcnt++;
+ mutex_unlock(&mem_id_lock);
+ return 0;
+ }
+ mutex_unlock(&mem_id_lock);
+
xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
if (!xdp_alloc)
return -ENOMEM;
@@ -360,6 +410,8 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
xdp_rxq->mem.id = id;
xdp_alloc->mem = xdp_rxq->mem;
xdp_alloc->allocator = allocator;
+ xdp_alloc->refcnt = 1;
+ xdp_alloc->queue_index = xdp_rxq->queue_index;
/* Insert allocator into ID lookup table */
ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
>> return 0;
>> }
>>
>> @@ -406,6 +409,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> xdp_alloc->mem = xdp_rxq->mem;
>> xdp_alloc->allocator = allocator;
>> xdp_alloc->refcnt = 1;
>> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>>
>> /* Insert allocator into ID lookup table */
>> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>>
>> Jesper, are you Ok with this version?
>
>Please see my other patch, this is based on our first refcnt attempt.
>I think that other patch is a better way forward.
XDP patch serves it better and can prevent not only obj deletion but also
pool flush. So I propose use 2 patches.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply related
* Re: [PATCH net-next v6 04/15] ethtool: introduce ethtool netlink interface
From: Michal Kubecek @ 2019-07-02 14:52 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190702122521.GN2250@nanopsycho>
On Tue, Jul 02, 2019 at 02:25:21PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:49:59PM CEST, mkubecek@suse.cz wrote:
> >+Request header
> >+--------------
> >+
> >+Each request or reply message contains a nested attribute with common header.
> >+Structure of this header is
>
> Missing ":"
OK
> >+
> >+ ETHTOOL_A_HEADER_DEV_INDEX (u32) device ifindex
> >+ ETHTOOL_A_HEADER_DEV_NAME (string) device name
> >+ ETHTOOL_A_HEADER_INFOMASK (u32) info mask
> >+ ETHTOOL_A_HEADER_GFLAGS (u32) flags common for all requests
> >+ ETHTOOL_A_HEADER_RFLAGS (u32) request specific flags
> >+
> >+ETHTOOL_A_HEADER_DEV_INDEX and ETHTOOL_A_HEADER_DEV_NAME identify the device
> >+message relates to. One of them is sufficient in requests, if both are used,
> >+they must identify the same device. Some requests, e.g. global string sets, do
> >+not require device identification. Most GET requests also allow dump requests
> >+without device identification to query the same information for all devices
> >+providing it (each device in a separate message).
> >+
> >+Optional info mask allows to ask only for a part of data provided by GET
>
> How this "infomask" works? What are the bits related to? Is that request
> specific?
The interpretation is request specific, the information returned for
a GET request is divided into multiple parts and client can choose to
request one of them (usually one). In the code so far, infomask bits
correspond to top level (nest) attributes but I would rather not make it
a strict rule.
I'll make the paragraph more verbose.
> >+request types. If omitted or zero, all data is returned. The two flag bitmaps
> >+allow enabling requestoptions; ETHTOOL_A_HEADER_GFLAGS are global flags common
>
> s/requestoptions;/request options./ ?
Yes.
> >+for all request types, flags recognized in ETHTOOL_A_HEADER_RFLAGS and their
> >+interpretation are specific for each request type. Global flags are
> >+
> >+ ETHTOOL_RF_COMPACT use compact format bitsets in reply
>
> Why "RF"? Isn't this "GF"? I would like "ETHTOOL_GFLAG_COMPACT" better.
RF as Request Flags. At the moment, global flags use ETHTOOL_RF_name
pattern and request specific flags ETHTOOL_RF_msgtype_name. GFLAG and
RFLAG would probably show the relation better, so how about
ETHTOOL_GFLAG_name for global
ETHTOOL_RFLAG_msgtype_name for request specific
> >+ ETHTOOL_RF_REPLY send optional reply (SET and ACT requests)
> >+
> >+Request specific flags are described with each request type. For both flag
> >+attributes, new flags should follow the general idea that if the flag is not
> >+set, the behaviour is the same as (or closer to) the behaviour before it was
>
> "closer to" ? That would be unfortunate I believe...
There may be situations where it cannot be exactly the same, e.g.
because the flag affects interpretation of an attribute which was
introduced together with the flag. How about "...the behaviour is
backward compatible"?
> >+List of message types
> >+---------------------
> >+
> >+All constants identifying message types use ETHTOOL_CMD_ prefix and suffix
> >+according to message purpose:
> >+
> >+ _GET userspace request to retrieve data
> >+ _SET userspace request to set data
> >+ _ACT userspace request to perform an action
> >+ _GET_REPLY kernel reply to a GET request
> >+ _SET_REPLY kernel reply to a SET request
> >+ _ACT_REPLY kernel reply to an ACT request
> >+ _NTF kernel notification
> >+
> >+"GET" requests are sent by userspace applications to retrieve device
> >+information. They usually do not contain any message specific attributes.
> >+Kernel replies with corresponding "GET_REPLY" message. For most types, "GET"
> >+request with NLM_F_DUMP and no device identification can be used to query the
> >+information for all devices supporting the request.
> >+
> >+If the data can be also modified, corresponding "SET" message with the same
> >+layout as "GET" reply is used to request changes. Only attributes where
>
> s/"GET" reply"/"GET_REPLY" ?
> Maybe better to emphasize that the "GET_REPLY" is the one corresponding
> with "SET". But perhaps I got this sentence all wrong :/
OK
> >+a change is requested are included in such request (also, not all attributes
> >+may be changed). Replies to most "SET" request consist only of error code and
> >+extack; if kernel provides additional data, it is sent in the form of
> >+corresponding "SET_REPLY" message (if ETHTOOL_RF_REPLY flag was set in request
> >+header).
> >+
> >+Data modification also triggers sending a "NTF" message with a notification.
> >+These usually bear only a subset of attributes which was affected by the
> >+change. The same notification is issued if the data is modified using other
> >+means (mostly ioctl ethtool interface). Unlike notifications from ethtool
> >+netlink code which are only sent if something actually changed, notifications
> >+triggered by ioctl interface may be sent even if the request did not actually
> >+change any data.
>
> Interesting. What's the reason for that?
Most setting commands in ioctl interface do not even query the original
state, they just pass the structure from ioctl() to ethtool_ops handler.
We could add retrieving the original state first but I suppose we would
still have to call the handler anyway even if requested values are the
same (as that's what kernel does now) and it's not clear if omitting the
notification in such case is the right thing to do.
> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> >index 3ebfab2bca66..f30e0da88be5 100644
> >--- a/net/ethtool/Makefile
> >+++ b/net/ethtool/Makefile
> >@@ -1,3 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> >-obj-y += ioctl.o
> >+obj-y += ioctl.o
> >+
> >+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>
> Hmm, I wonder, why not to make this always on? We want users to use
> it, memory savings in case it is off would be minimal. RTNetlink is also
> always on. Ethtool ioctl is also always on.
We have already discussed this in the previous version. Someone claimed
earlier that building a kernel without ethtool interface would make
sense for some minimalistic systems. My plan is to make the ioctl
interface also optional once it's possible for (sufficiently new)
ethtool to work without it.
Michal
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 14:52 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap, brouer
In-Reply-To: <20190702144426.GD4510@khorivan>
On Tue, 2 Jul 2019 17:44:27 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >
> >Jesper recently removed page_pool_destroy() (from driver invocation) and
> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
> >handle in-flight packets/pages. This created an asymmetry in drivers
> >create/destroy pairs.
> >
> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
> >This serves two purposes, (1) simplify drivers error handling as driver now
> >drivers always calls page_pool_destroy() and don't need to track if
> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
> >where a single RX-queue (with a single page_pool) provides packets for two
> >net_device'es, and thus needs to register the same page_pool twice with two
> >xdp_rxq_info structures.
>
> As I tend to use xdp level patch there is no more reason to mention (2) case
> here. XDP patch serves it better and can prevent not only obj deletion but also
> pool flush, so, this one patch I could better leave only for (1) case.
I don't understand what you are saying.
Do you approve this patch, or do you reject this patch?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Jesper Dangaard Brouer @ 2019-07-02 14:46 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190702102700.GA4510@khorivan>
On Tue, 2 Jul 2019 13:27:07 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Mon, Jul 01, 2019 at 01:40:59PM +0200, Jesper Dangaard Brouer wrote:
> >
> >I'm very skeptical about this approach.
> >
> >On Sun, 30 Jun 2019 20:23:43 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> XDP rxqs can be same for ndevs running under same rx napi softirq.
> >> But there is no ability to register same allocator for both rxqs,
> >> by fact it can same rxq but has different ndev as a reference.
> >
> >This description is not very clear. It can easily be misunderstood.
> >
> >It is an absolute requirement that each RX-queue have their own
> >page_pool object/allocator. (This where the performance comes from) as
> >the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
> >
> >Your driver/hardware seems to have special case, where a single
> >RX-queue can receive packets for two different net_device'es.
> >
> >Do you violate this XDP devmap redirect assumption[1]?
> >[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
> Seems that yes, but that's used only for trace for now.
> As it runs under napi and flush clear dev_rx i must do it right in the
> rx_handler. So next patchset version will have one patch less.
>
> Thanks!
>
> >
> >
> >> Due to last changes allocator destroy can be defered till the moment
> >> all packets are recycled by destination interface, afterwards it's
> >> freed. In order to schedule allocator destroy only after all users are
> >> unregistered, add refcnt to allocator object and schedule to destroy
> >> only it reaches 0.
> >
> >The guiding principles when designing an API, is to make it easy to
> >use, but also make it hard to misuse.
> >
> >Your API change makes it easy to misuse the API. As it make it easy to
> >(re)use the allocator pointer (likely page_pool) for multiple
> >xdp_rxq_info structs. It is only valid for your use-case, because you
> >have hardware where a single RX-queue delivers to two different
> >net_devices. For other normal use-cases, this will be a violation.
> >
> >If I was a user of this API, and saw your xdp_allocator_get(), then I
> >would assume that this was the normal case. As minimum, we need to add
> >a comment in the code, about this specific/intended use-case. I
> >through about detecting the misuse, by adding a queue_index to
> >xdp_mem_allocator, that can be checked against, when calling
> >xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
> >the obvious mistake where queue_index mismatch).
>
> I can add, but not sure if it has or can have some conflicts with another
> memory allocators, now or in future. Main here to not became a cornerstone
> in some not obvious use-cases.
>
> So, for now, let it be in this way:
>
> --- a/include/net/xdp_priv.h
> +++ b/include/net/xdp_priv.h
> @@ -19,6 +19,7 @@ struct xdp_mem_allocator {
> struct delayed_work defer_wq;
> unsigned long defer_warn;
> unsigned long refcnt;
> + u32 queue_index;
> };
>
> #endif /* __LINUX_NET_XDP_PRIV_H__ */
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index a44621190fdc..c4bf29810f4d 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -324,7 +324,7 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
> return true;
> }
>
> -static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
> {
> struct xdp_mem_allocator *xae, *xa = NULL;
> struct rhashtable_iter iter;
> @@ -336,7 +336,6 @@ static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>
> while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
> if (xae->allocator == allocator) {
> - xae->refcnt++;
> xa = xae;
> break;
> }
> @@ -386,9 +385,13 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> }
> }
>
> - xdp_alloc = xdp_allocator_get(allocator);
> + xdp_alloc = xdp_allocator_find(allocator);
> if (xdp_alloc) {
> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
> + return -EINVAL;
> +
> xdp_rxq->mem.id = xdp_alloc->mem.id;
> + xdp_alloc->refcnt++;
This is now adjusted outside lock, not good.
> return 0;
> }
>
> @@ -406,6 +409,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> xdp_alloc->mem = xdp_rxq->mem;
> xdp_alloc->allocator = allocator;
> xdp_alloc->refcnt = 1;
> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>
> /* Insert allocator into ID lookup table */
> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
> Jesper, are you Ok with this version?
Please see my other patch, this is based on our first refcnt attempt.
I think that other patch is a better way forward.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 14:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <156207778364.29180.5111562317930943530.stgit@firesoul>
On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
>From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>
>Jesper recently removed page_pool_destroy() (from driver invocation) and
>moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
>handle in-flight packets/pages. This created an asymmetry in drivers
>create/destroy pairs.
>
>This patch add page_pool user refcnt and reintroduce page_pool_destroy.
>This serves two purposes, (1) simplify drivers error handling as driver now
>drivers always calls page_pool_destroy() and don't need to track if
>xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
>where a single RX-queue (with a single page_pool) provides packets for two
>net_device'es, and thus needs to register the same page_pool twice with two
>xdp_rxq_info structures.
As I tend to use xdp level patch there is no more reason to mention (2) case
here. XDP patch serves it better and can prevent not only obj deletion but also
pool flush, so, this one patch I could better leave only for (1) case.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH][next] wil6210: fix wil_cid_valid with negative cid values
From: Colin King @ 2019-07-02 14:40 UTC (permalink / raw)
To: Maya Erez, Kalle Valo, David S . Miller, linux-wireless, wil6210,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There are several occasions where a negative cid value is passed
into wil_cid_valid and this is converted into a u8 causing the
range check of cid >= 0 to always succeed. Fix this by making
the cid argument an int to handle any -ve error value of cid.
An example of this behaviour is in wil_cfg80211_dump_station,
where cid is assigned -ENOENT if the call to wil_find_cid_by_idx
fails, and this -ve value is passed to wil_cid_valid. I believe
that the conversion of -ENOENT to the u8 value 254 which is
greater than wil->max_assoc_sta causes wil_find_cid_by_idx to
currently work fine, but I think is by luck and not the
intended behaviour.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/ath/wil6210/wil6210.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 6f456b311a39..25a1adcb38eb 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -1144,7 +1144,7 @@ static inline void wil_c(struct wil6210_priv *wil, u32 reg, u32 val)
/**
* wil_cid_valid - check cid is valid
*/
-static inline bool wil_cid_valid(struct wil6210_priv *wil, u8 cid)
+static inline bool wil_cid_valid(struct wil6210_priv *wil, int cid)
{
return (cid >= 0 && cid < wil->max_assoc_sta);
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] User mode linux bump maximum MTU tuntap interface [RESAND]
From: Richard Weinberger @ 2019-07-02 14:40 UTC (permalink / raw)
To: Алексей
Cc: netdev, linux-um, Anton Ivanov
In-Reply-To: <fc526c78-2d3f-90ca-8317-a89eb653cbf9@yandex.ru>
CC'ing um folks.
On Tue, Jul 2, 2019 at 3:01 PM Алексей <ne-vlezay80@yandex.ru> wrote:
>
> Hello, the parameter ETH_MAX_PACKET limited to 1500 bytes is the not
> support jumbo frames.
>
> This patch change ETH_MAX_PACKET the 65535 bytes to jumbo frame support
> with user mode linux tuntap driver.
>
>
> PATCH:
>
> -------------------
>
>
> diff -ruNP ../linux_orig/linux-5.1/arch/um/include/shared/net_user.h
> ./arch/um/include/shared/net_user.h
> --- a/arch/um/include/shared/net_user.h 2019-05-06 00:42:58.000000000
> +0000
> +++ b/arch/um/include/shared/net_user.h 2019-07-02 07:14:13.593333356
> +0000
> @@ -9,7 +9,7 @@
> #define ETH_ADDR_LEN (6)
> #define ETH_HEADER_ETHERTAP (16)
> #define ETH_HEADER_OTHER (26) /* 14 for ethernet + VLAN + MPLS for
> crazy people */
> -#define ETH_MAX_PACKET (1500)
> +#define ETH_MAX_PACKET (65535)
>
> #define UML_NET_VERSION (4)
>
> -------------------
>
>
--
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH net-next 5/5] ipv4: use indirect call wrappers for {tcp,udp}_{recv,send}msg()
From: Willem de Bruijn @ 2019-07-02 14:38 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, David S. Miller
In-Reply-To: <a2806dab7e472b5316da87318f1b8e48ff68cd4b.camel@redhat.com>
On Tue, Jul 2, 2019 at 9:03 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2019-07-01 at 15:07 -0400, Willem de Bruijn wrote:
> > On Mon, Jul 1, 2019 at 1:10 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > This avoids an indirect call per syscall for common ipv4 transports
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > net/ipv4/af_inet.c | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> > > index 8421e2f5bbb3..9a2f17d0c5f5 100644
> > > --- a/net/ipv4/af_inet.c
> > > +++ b/net/ipv4/af_inet.c
> > > @@ -797,6 +797,8 @@ int inet_send_prepare(struct sock *sk)
> > > }
> > > EXPORT_SYMBOL_GPL(inet_send_prepare);
> > >
> > > +INDIRECT_CALLABLE_DECLARE(int udp_sendmsg(struct sock *, struct msghdr *,
> > > + size_t));
> >
> > Small nit: this is already defined in include/net/udp.h, which is
> > included. So like tcp_sendmsg, probably no need to declare.
>
> Thank you for the review!
>
> You are right, that declaration can be dropped.
> >
> > If defining inet6_sendmsg and inet6_recvmsg in include/net/ipv6.h,
> > perhaps do the same for the other missing functions, instead of these
> > indirect declarations at the callsite?
>
> Uhm... since inet6_{send,recv}msg exists only for retpoline sake and
> are not exported, I think is probably better move their declaration to
> socket.c via INDIRECT_CALLABLE_DECLARE(), to that ICWs are all self-
> contained.
>
> Unless there are objections about spamming, I can repost the series
> with the above changes.
If just for the spurious declaration, can be merged as is, too.
Either way. Not spammy at all to resend after a day.
^ permalink raw reply
* [PATCH bpf] xdp: fix race on generic receive path
From: Ilya Maximets @ 2019-07-02 14:36 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Ilya Maximets
In-Reply-To: <CGME20190702143639eucas1p2b168c68c35b70aac75cad6c72ccc81ad@eucas1p2.samsung.com>
Unlike driver mode, generic xdp receive could be triggered
by different threads on different CPU cores at the same time
leading to the fill and rx queue breakage. For example, this
could happen while sending packets from two processes to the
first interface of veth pair while the second part of it is
open with AF_XDP socket.
Need to take a lock for each generic receive to avoid race.
Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
include/net/xdp_sock.h | 2 ++
net/xdp/xsk.c | 32 +++++++++++++++++++++++---------
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index d074b6d60f8a..ac3c047d058c 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -67,6 +67,8 @@ struct xdp_sock {
* in the SKB destructor callback.
*/
spinlock_t tx_completion_lock;
+ /* Protects generic receive. */
+ spinlock_t rx_lock;
u64 rx_dropped;
};
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index a14e8864e4fa..19f41d2b670c 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -119,17 +119,22 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
{
u32 metalen = xdp->data - xdp->data_meta;
u32 len = xdp->data_end - xdp->data;
+ unsigned long flags;
void *buffer;
u64 addr;
int err;
- if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
- return -EINVAL;
+ spin_lock_irqsave(&xs->rx_lock, flags);
+
+ if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
+ err = -EINVAL;
+ goto out_unlock;
+ }
if (!xskq_peek_addr(xs->umem->fq, &addr) ||
len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
- xs->rx_dropped++;
- return -ENOSPC;
+ err = -ENOSPC;
+ goto out_drop;
}
addr += xs->umem->headroom;
@@ -138,13 +143,21 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
memcpy(buffer, xdp->data_meta, len + metalen);
addr += metalen;
err = xskq_produce_batch_desc(xs->rx, addr, len);
- if (!err) {
- xskq_discard_addr(xs->umem->fq);
- xsk_flush(xs);
- return 0;
- }
+ if (err)
+ goto out_drop;
+
+ xskq_discard_addr(xs->umem->fq);
+ xskq_produce_flush_desc(xs->rx);
+ spin_unlock_irqrestore(&xs->rx_lock, flags);
+
+ xs->sk.sk_data_ready(&xs->sk);
+ return 0;
+
+out_drop:
xs->rx_dropped++;
+out_unlock:
+ spin_unlock_irqrestore(&xs->rx_lock, flags);
return err;
}
@@ -765,6 +778,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
xs = xdp_sk(sk);
mutex_init(&xs->mutex);
+ spin_lock_init(&xs->rx_lock);
spin_lock_init(&xs->tx_completion_lock);
mutex_lock(&net->xdp.lock);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt
From: Willem de Bruijn @ 2019-07-02 14:35 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: Sabrina Dubroca, Network Development
In-Reply-To: <b393fc9f9a9e4e49b9cbe6edebb3cd38301ffd92.camel@domdv.de>
On Tue, Jul 2, 2019 at 12:38 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> Ouch, I missed that when Andreas sent me that patch before. No, it is
> > actually intended. If we skip macsec_decrypt(), we should still
> > account for that packet in the InPktsUnchecked/InPktsDelayed
> > counters. That's in Figure 10-5 in the standard.
> >
> > Thanks for catching this, Willem. That patch should only move the
> > IS_ERR(skb) case under the block where macsec_decrypt() is called, but
> > not move the call to macsec_post_decrypt().
>
> Updated patch below.
>
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
When making a change in a patch set, please resubmit the entire
patchset (with a v2, and record the changelog).
^ permalink raw reply
* Re: [PATCH net 2/2] macsec: fix checksumming after decryption
From: Willem de Bruijn @ 2019-07-02 14:35 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: Network Development, Sabrina Dubroca
In-Reply-To: <94382bd8cfbf924779ce86cd6405331f70f65c27.camel@domdv.de>
On Tue, Jul 2, 2019 at 12:25 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> On Sun, 2019-06-30 at 21:47 -0400, Willem de Bruijn wrote:
> > On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@domdv.de>
> > wrote:
> > > Fix checksumming after decryption.
> > >
> > > Signed-off-by: Andreas Steinmetz <ast@domdv.de>
> > >
> > > --- a/drivers/net/macsec.c 2019-06-30 22:14:10.250285314 +0200
> > > +++ b/drivers/net/macsec.c 2019-06-30 22:15:11.931230417 +0200
> > > @@ -869,6 +869,7 @@
> > >
> > > static void macsec_finalize_skb(struct sk_buff *skb, u8 icv_len,
> > > u8 hdr_len)
> > > {
> > > + skb->ip_summed = CHECKSUM_NONE;
> > > memmove(skb->data + hdr_len, skb->data, 2 * ETH_ALEN);
> > > skb_pull(skb, hdr_len);
> > > pskb_trim_unique(skb, skb->len - icv_len);
> >
> > Does this belong in macset_reset_skb?
>
> Putting this in macsec_reset_skb would then miss out the "nosci:" part
> of the RX path in macsec_handle_frame().
It is called on each nskb before calling netif_rx.
It indeed is not called when returning RX_HANDLER_PASS, but that is correct?
^ permalink raw reply
* [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 14:31 UTC (permalink / raw)
To: netdev, Ilias Apalodimas, ivan.khoronzhuk, Jesper Dangaard Brouer
Cc: grygorii.strashko, jakub.kicinski, daniel, john.fastabend, ast,
linux-kernel, linux-omap
In-Reply-To: <20190702153902.0e42b0b2@carbon>
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Jesper recently removed page_pool_destroy() (from driver invocation) and
moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
handle in-flight packets/pages. This created an asymmetry in drivers
create/destroy pairs.
This patch add page_pool user refcnt and reintroduce page_pool_destroy.
This serves two purposes, (1) simplify drivers error handling as driver now
drivers always calls page_pool_destroy() and don't need to track if
xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
where a single RX-queue (with a single page_pool) provides packets for two
net_device'es, and thus needs to register the same page_pool twice with two
xdp_rxq_info structures.
This patch is a modified version of Ivan Khoronzhuk's original patch.
Thus, Jesper gives author ownership to Ivan.
Link: https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
To Ivan,
If you agree with this patch, please add your Signed-off-by.
You can also say if you prefer to take this patch and make it
part of your driver patchset, what ever you prefer.
--Jesper
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 6 ++---
drivers/net/ethernet/socionext/netsec.c | 8 ++----
include/net/page_pool.h | 27 +++++++++++++++++++++
net/core/page_pool.c | 8 ++++++
net/core/xdp.c | 3 ++
5 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 1085040675ae..ce1c7a449eae 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -545,10 +545,8 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
}
err = xdp_rxq_info_reg_mem_model(&rq->xdp_rxq,
MEM_TYPE_PAGE_POOL, rq->page_pool);
- if (err) {
- page_pool_free(rq->page_pool);
+ if (err)
goto err_free;
- }
for (i = 0; i < wq_sz; i++) {
if (rq->wq_type == MLX5_WQ_TYPE_LINKED_LIST_STRIDING_RQ) {
@@ -613,6 +611,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
if (rq->xdp_prog)
bpf_prog_put(rq->xdp_prog);
xdp_rxq_info_unreg(&rq->xdp_rxq);
+ page_pool_destroy(rq->page_pool);
mlx5_wq_destroy(&rq->wq_ctrl);
return err;
@@ -643,6 +642,7 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
}
xdp_rxq_info_unreg(&rq->xdp_rxq);
+ page_pool_destroy(rq->page_pool);
mlx5_wq_destroy(&rq->wq_ctrl);
}
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 5544a722543f..43ab0ce90704 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1210,15 +1210,11 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id)
}
}
- /* Rx is currently using page_pool
- * since the pool is created during netsec_setup_rx_dring(), we need to
- * free the pool manually if the registration failed
- */
+ /* Rx is currently using page_pool */
if (id == NETSEC_RING_RX) {
if (xdp_rxq_info_is_reg(&dring->xdp_rxq))
xdp_rxq_info_unreg(&dring->xdp_rxq);
- else
- page_pool_free(dring->page_pool);
+ page_pool_destroy(dring->page_pool);
}
memset(dring->desc, 0, sizeof(struct netsec_desc) * DESC_NUM);
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index ee9c871d2043..ea974856d0f7 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -101,6 +101,14 @@ struct page_pool {
struct ptr_ring ring;
atomic_t pages_state_release_cnt;
+
+ /* A page_pool is strictly tied to a single RX-queue being
+ * protected by NAPI, due to above pp_alloc_cache. This
+ * refcnt serves two purposes. (1) simplify drivers error
+ * handling, and (2) allow special cases where a single
+ * RX-queue provides packet for two net_device'es.
+ */
+ refcount_t user_cnt;
};
struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
@@ -134,6 +142,15 @@ static inline void page_pool_free(struct page_pool *pool)
#endif
}
+/* Drivers use this instead of page_pool_free */
+static inline void page_pool_destroy(struct page_pool *pool)
+{
+ if (!pool)
+ return;
+
+ page_pool_free(pool);
+}
+
/* Never call this directly, use helpers below */
void __page_pool_put_page(struct page_pool *pool,
struct page *page, bool allow_direct);
@@ -201,4 +218,14 @@ static inline bool is_page_pool_compiled_in(void)
#endif
}
+static inline void page_pool_get(struct page_pool *pool)
+{
+ refcount_inc(&pool->user_cnt);
+}
+
+static inline bool page_pool_put(struct page_pool *pool)
+{
+ return refcount_dec_and_test(&pool->user_cnt);
+}
+
#endif /* _NET_PAGE_POOL_H */
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index b366f59885c1..3272dc7a8c81 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -49,6 +49,9 @@ static int page_pool_init(struct page_pool *pool,
atomic_set(&pool->pages_state_release_cnt, 0);
+ /* Driver calling page_pool_create() also call page_pool_destroy() */
+ refcount_set(&pool->user_cnt, 1);
+
if (pool->p.flags & PP_FLAG_DMA_MAP)
get_device(pool->p.dev);
@@ -70,6 +73,7 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
kfree(pool);
return ERR_PTR(err);
}
+
return pool;
}
EXPORT_SYMBOL(page_pool_create);
@@ -356,6 +360,10 @@ static void __warn_in_flight(struct page_pool *pool)
void __page_pool_free(struct page_pool *pool)
{
+ /* Only last user actually free/release resources */
+ if (!page_pool_put(pool))
+ return;
+
WARN(pool->alloc.count, "API usage violation");
WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty");
diff --git a/net/core/xdp.c b/net/core/xdp.c
index b29d7b513a18..e57a0eb1feb7 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -372,6 +372,9 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
mutex_unlock(&mem_id_lock);
+ if (type == MEM_TYPE_PAGE_POOL)
+ page_pool_get(xdp_alloc->page_pool);
+
trace_mem_connect(xdp_alloc, xdp_rxq);
return 0;
err:
^ permalink raw reply related
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Nikolay Aleksandrov @ 2019-07-02 14:27 UTC (permalink / raw)
To: Linus Lüssing, Ido Schimmel
Cc: Russell King - ARM Linux admin, Ido Schimmel, Vivien Didelot,
Florian Fainelli, netdev@vger.kernel.org, Jiri Pirko,
andrew@lunn.ch, davem@davemloft.net, bridge, b.a.t.m.a.n
In-Reply-To: <20190630165601.GC2500@otheros>
On 30/06/2019 19:56, Linus Lüssing wrote:
> On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote:
>> I would like to avoid having drivers take the querier state into account
>> as it will only complicate things further.
>
> I absolutely share your pain. Initially in the early prototypes of
> multicast awareness in batman-adv we did not consider the querier state.
> And doing so later did indeed complicate the code a good bit in batman-adv
> (together with the IGMP/MLD suppression issues). I would have loved to
> avoid that.
>
>
>> Is there anything we can do about it? Enable the bridge querier if no
>> other querier was detected? Commit c5c23260594c ("bridge: Add
>> multicast_querier toggle and disable queries by default") disabled
>> queries by default, but I'm only suggesting to turn them on if no other
>> querier was detected on the link. Do you think it's still a problem?
>
> As soon as you start becoming the querier, you will not be able to reliably
> detect anymore whether you are the only querier candidate.
>
> If any random Linux host using a bridge device were potentially becoming
> a querier, that would cause quite some trouble when this host is
> behind some bad, bottleneck connection. This host will receive
> all multicast traffic, not just IGMP/MLD reports. And with a
> congested connection and then unreliable IGMP/MLD, multicast would
> become unreliable overall in this domain. So it's important that
> your querier is not running in the "dark, remote, dusty closet" of
> your network (topologically speaking).
>
+1
We definitely don't want random hosts becoming queriers
>> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
>>> See commit b00589af3b04 ("bridge: disable snooping if there is no
>>> querier"). I think that's unfortunate behavior that we need because
>>> multicast snooping is enabled by default. If it weren't enabled by
>>> default, then anyone enabling it would also make sure there's a querier
>>> in the network.
>
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.
>
Indeed, also you could create the bridge with explicit mcast parameters if you need
different behaviour on start. Unfortunately I think you'll have to handle
the querier state.
>
> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
>
> But of course, updating IETF standards is no solution for now.
>
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right? So similarly, you could react to appearing or
> disappearing queriers?
>
> Cheers, Linus
>
Thanks,
Nik
^ permalink raw reply
* Re: [PATCH v5 net-next 6/6] net: ethernet: ti: cpsw: add XDP support
From: Ivan Khoronzhuk @ 2019-07-02 14:24 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, davem, ast, linux-kernel, linux-omap,
ilias.apalodimas, netdev, daniel, jakub.kicinski, john.fastabend
In-Reply-To: <20190702153902.0e42b0b2@carbon>
On Tue, Jul 02, 2019 at 03:39:02PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 14:37:39 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Mon, Jul 01, 2019 at 06:19:01PM +0200, Jesper Dangaard Brouer wrote:
>> >On Sun, 30 Jun 2019 20:23:48 +0300
>> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >
>> >> +static int cpsw_ndev_create_xdp_rxq(struct cpsw_priv *priv, int ch)
>> >> +{
>> >> + struct cpsw_common *cpsw = priv->cpsw;
>> >> + int ret, new_pool = false;
>> >> + struct xdp_rxq_info *rxq;
>> >> +
>> >> + rxq = &priv->xdp_rxq[ch];
>> >> +
>> >> + ret = xdp_rxq_info_reg(rxq, priv->ndev, ch);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + if (!cpsw->page_pool[ch]) {
>> >> + ret = cpsw_create_rx_pool(cpsw, ch);
>> >> + if (ret)
>> >> + goto err_rxq;
>> >> +
>> >> + new_pool = true;
>> >> + }
>> >> +
>> >> + ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL,
>> >> + cpsw->page_pool[ch]);
>> >> + if (!ret)
>> >> + return 0;
>> >> +
>> >> + if (new_pool) {
>> >> + page_pool_free(cpsw->page_pool[ch]);
>> >> + cpsw->page_pool[ch] = NULL;
>> >> + }
>> >> +
>> >> +err_rxq:
>> >> + xdp_rxq_info_unreg(rxq);
>> >> + return ret;
>> >> +}
>> >
>> >Looking at this, and Ilias'es XDP-netsec error handling path, it might
>> >be a mistake that I removed page_pool_destroy() and instead put the
>> >responsibility on xdp_rxq_info_unreg().
>>
>> As for me this is started not from page_pool_free, but rather from calling
>> unreg_mem_model from rxq_info_unreg. Then, if page_pool_free is hidden
>> it looks more a while normal to move all chain to be self destroyed.
>>
>> >
>> >As here, we have to detect if page_pool_create() was a success, and then
>> >if xdp_rxq_info_reg_mem_model() was a failure, explicitly call
>> >page_pool_free() because the xdp_rxq_info_unreg() call cannot "free"
>> >the page_pool object given it was not registered.
>>
>> Yes, it looked a little bit ugly from the beginning, but, frankly,
>> I have got used to this already.
>>
>> >
>> >Ivan's patch in[1], might be a better approach, which forced all
>> >drivers to explicitly call page_pool_free(), even-though it just
>> >dec-refcnt and the real call to page_pool_free() happened via
>> >xdp_rxq_info_unreg().
>> >
>> >To better handle error path, I would re-introduce page_pool_destroy(),
>>
>> So, you might to do it later as I understand, and not for my special
>> case but becouse it makes error path to look a little bit more pretty.
>> I'm perfectly fine with this, and better you add this, for now my
>> implementation requires only "xdp: allow same allocator usage" patch,
>> but if you insist I can resend also patch in question afterwards my
>> series is applied (with modification to cpsw & netsec & mlx5 & page_pool).
>>
>> What's your choice? I can add to your series patch needed for cpsw to
>> avoid some misuse.
>
>I will try to create a cleaned-up version of your patch[1] and
>re-introduce page_pool_destroy() for drivers to use, then we can build
>your driver on top of that.
I've corrected patch to xdp core and tested. The "page pool API" change
seems is orthogonal now. So no limits to send v6 that is actually done
and no more strict dependency on page pool API changes whenever that
can happen.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH -next] carl9170: remove set but not used variable 'udev'
From: YueHaibing @ 2019-07-02 14:12 UTC (permalink / raw)
To: chunkeey; +Cc: linux-kernel, netdev, linux-wireless, kvalo, davem, YueHaibing
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/wireless/ath/carl9170/usb.c: In function carl9170_usb_disconnect:
drivers/net/wireless/ath/carl9170/usb.c:1110:21:
warning: variable udev set but not used [-Wunused-but-set-variable]
It is not use since commit feb09b293327 ("carl9170:
fix misuse of device driver API")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/ath/carl9170/usb.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index 99f1897..486957a 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -1107,12 +1107,10 @@ static int carl9170_usb_probe(struct usb_interface *intf,
static void carl9170_usb_disconnect(struct usb_interface *intf)
{
struct ar9170 *ar = usb_get_intfdata(intf);
- struct usb_device *udev;
if (WARN_ON(!ar))
return;
- udev = ar->udev;
wait_for_completion(&ar->fw_load_wait);
if (IS_INITIALIZED(ar)) {
--
2.7.4
^ permalink raw reply related
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
From: Yuya Kusakabe @ 2019-07-02 14:11 UTC (permalink / raw)
To: Jason Wang
Cc: ast, daniel, davem, hawk, jakub.kicinski, john.fastabend, kafai,
mst, netdev, songliubraving, yhs
In-Reply-To: <ca724dcf-4ffb-ff49-d307-1b45143712b5@redhat.com>
On 7/2/19 5:33 PM, Jason Wang wrote:
>
> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>> This adds XDP meta data support to both receive_small() and
>> receive_mergeable().
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> v3:
>> - fix preserve the vnet header in receive_small().
>> v2:
>> - keep copy untouched in page_to_skb().
>> - preserve the vnet header in receive_small().
>> - fix indentation.
>> ---
>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> - if (hdr_valid)
>> + if (hdr_valid && !metasize)
>> memcpy(hdr, p, hdr_len);
>> len -= hdr_len;
>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> copy = skb_tailroom(skb);
>> skb_put_data(skb, p, copy);
>> + if (metasize) {
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + }
>> +
>> len -= copy;
>> offset += copy;
>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> + /* Copy the vnet header to the front of data_hard_start to avoid
>> + * overwriting by XDP meta data */
>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
>
>
> What happens if we have a large metadata that occupies all headroom here?
>
> Thanks
Do you mean a large "XDP" metadata? If a large metadata is a large "XDP" metadata, I think we can not use a metadata that occupies all headroom. The size of metadata limited by bpf_xdp_adjust_meta() as below.
bpf_xdp_adjust_meta() in net/core/filter.c:
if (unlikely((metalen & (sizeof(__u32) - 1)) ||
(metalen > 32)))
return -EACCES;
Thanks.
>
>
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> stats->xdp_packets++;
>> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> skb_reserve(skb, headroom - delta);
>> skb_put(skb, len);
>> if (!delta) {
>> - buf += header_offset;
>> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> struct virtnet_rq_stats *stats)
>> {
>> struct page *page = buf;
>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + struct sk_buff *skb =
>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjustments. Note other cases do not build an
>> * skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>> /* recalculate len if xdp.data or xdp.data_end were
>> * adjusted
>> @@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> - head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>> + len, PAGE_SIZE, false,
>> + metasize);
>> return head_skb;
>> }
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/6] xsk: add support for need_wakeup flag in AF_XDP rings
From: Magnus Karlsson @ 2019-07-02 13:58 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: Magnus Karlsson, ast@kernel.org, bjorn.topel@intel.com,
daniel@iogearbox.net, netdev@vger.kernel.org, brouer@redhat.com,
bpf@vger.kernel.org, bruce.richardson@intel.com,
ciara.loftus@intel.com, jakub.kicinski@netronome.com,
xiaolong.ye@intel.com, qi.z.zhang@intel.com,
sridhar.samudrala@intel.com, kevin.laatz@intel.com,
ilias.apalodimas@linaro.org, kiran.patil@intel.com,
axboe@kernel.dk, maciej.fijalkowski@intel.com,
maciejromanfijalkowski@gmail.com,
intel-wired-lan@lists.osuosl.org
In-Reply-To: <d4318783-18a4-d5c1-1044-691aaebb2b0a@mellanox.com>
On Tue, Jul 2, 2019 at 3:47 PM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> On 2019-07-02 12:21, Magnus Karlsson wrote:
> >
> > +/* XDP_RING flags */
> > +#define XDP_RING_NEED_WAKEUP (1 << 0)
> > +
> > struct xdp_ring_offset {
> > __u64 producer;
> > __u64 consumer;
> > __u64 desc;
> > + __u64 flags;
> > };
> >
> > struct xdp_mmap_offsets {
>
> <snip>
>
> > @@ -621,9 +692,12 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> > case XDP_MMAP_OFFSETS:
> > {
> > struct xdp_mmap_offsets off;
> > + bool flags_supported = true;
> >
> > - if (len < sizeof(off))
> > + if (len < sizeof(off) - sizeof(off.rx.flags))
> > return -EINVAL;
> > + else if (len < sizeof(off))
> > + flags_supported = false;
> >
> > off.rx.producer = offsetof(struct xdp_rxtx_ring, ptrs.producer);
> > off.rx.consumer = offsetof(struct xdp_rxtx_ring, ptrs.consumer);
> > @@ -638,6 +712,16 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> > off.cr.producer = offsetof(struct xdp_umem_ring, ptrs.producer);
> > off.cr.consumer = offsetof(struct xdp_umem_ring, ptrs.consumer);
> > off.cr.desc = offsetof(struct xdp_umem_ring, desc);
> > + if (flags_supported) {
> > + off.rx.flags = offsetof(struct xdp_rxtx_ring,
> > + ptrs.flags);
> > + off.tx.flags = offsetof(struct xdp_rxtx_ring,
> > + ptrs.flags);
> > + off.fr.flags = offsetof(struct xdp_umem_ring,
> > + ptrs.flags);
> > + off.cr.flags = offsetof(struct xdp_umem_ring,
> > + ptrs.flags);
> > + }
>
> As far as I understood (correct me if I'm wrong), you are trying to
> preserve backward compatibility, so that if userspace doesn't support
> the flags field, you will determine that by looking at len and fall back
> to the old format.
That was the intention yes.
> However, two things are broken here:
>
> 1. The check `len < sizeof(off) - sizeof(off.rx.flags)` should be `len <
> sizeof(off) - 4 * sizeof(flags)`, because struct xdp_mmap_offsets
> consists of 4 structs xdp_ring_offset.
>
> 2. The old and new formats are not binary compatible, as flags are
> inserted in the middle of struct xdp_mmap_offsets.
You are correct. Since there are four copies of the xdp_ring_offset
this simple scheme will not work. I will instead create an internal
version 1 of the struct that I fill in and pass to user space if I
detect that user space is asking for the v1 size.
Thanks for catching Maxim. Keep'em coming.
/Magnus
^ permalink raw reply
* Re: Memory leaks in IPv6 ndisc on v4.19.56
From: Martin Weinelt @ 2019-07-02 13:54 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
In-Reply-To: <8fadeb22-2a9b-6038-01f9-bf32b5055965@linuxlounge.net>
[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]
Hi again,
another backtrace just came up.
Best,
Martin
$ ./scripts/faddr2line /usr/lib/debug/lib/modules/4.19.56/vmlinux build_skb+0x11/0x80
build_skb+0x11/0x80:
build_skb at net/core/skbuff.c:314
$ ./scripts/faddr2line /usr/lib/debug/lib/modules/4.19.56/kernel/drivers/net/tun.ko tun_build_skb.isra.56+0x191/0x4d0
tun_build_skb.isra.56+0x191/0x4d0:
tun_build_skb at /home/hexa/git/linux-stable/drivers/net/tun.c:1687
On 7/2/19 3:05 PM, Martin Weinelt wrote:
> Hi everyone,
>
> I've been experiencing memory leaks on the v4.19 series. I've started
> seeing them on Debian with v4.19.16 and I can reproduce them on v4.19.56
> using Debians kernel config. I was unable to reproduce this on
> v5.2.0-rc6/rc7.
>
> [ 1899.380321] kmemleak: 1138 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> On the machines in question we're running routers for a mesh networking
> setup based on the batman-adv kmod. Our setup consists of KVM guests
> running Debian, with each router having 18 bridges with the following
> master/slave relationship:
>
> bridge -> batman-adv -> {L2 tunnel, virtio net device}
>
> I've attached the output of kmemleak and I've looked up the top-most
> function offsets below:
>
> Best,
> Martin
>
>
> $ ./faddr2line /usr/lib/debug/lib/modules/4.19.56/vmlinux ndisc_recv_ns+0x356/0x5f0
> ndisc_recv_ns+0x356/0x5f0:
> __neigh_lookup at include/net/neighbour.h:513
> (inlined by) ndisc_recv_ns at net/ipv6/ndisc.c:916
>
> $ ./faddr2line /usr/lib/debug/lib/modules/4.19.56/vmlinux ndisc_router_discovery+0x4ab/0xae0
> ndisc_router_discovery+0x4ab/0xae0:
> __neigh_lookup at include/net/neighbour.h:513
> (inlined by) ndisc_router_discovery at net/ipv6/ndisc.c:1387
>
> $ ./faddr2line /usr/lib/debug/lib/modules/4.19.56/vmlinux ndisc_recv_rs+0x173/0x1b0
> ndisc_recv_rs+0x173/0x1b0:
> ndisc_recv_rs at net/ipv6/ndisc.c:1095
>
> $ ./faddr2line /usr/lib/debug/lib/modules/4.19.56/vmlinux ip6_finish_output2+0x211/0x570
> ip6_finish_output2+0x211/0x570:
> ip6_finish_output2 at net/ipv6/ip6_output.c:117
>
[-- Attachment #2: kmemleak2.log --]
[-- Type: text/x-log, Size: 1643 bytes --]
unreferenced object 0xffff98372c4c7900 (size 232):
comm "softirq", pid 0, jiffies 4296767615 (age 1191.500s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 10 f7 54 37 98 ff ff 00 00 00 00 00 00 00 00 ...T7...........
backtrace:
[<00000000857c2a4c>] build_skb+0x11/0x80
[<0000000076f6d169>] tun_build_skb.isra.56+0x191/0x4d0 [tun]
[<00000000c88dc3b6>] tun_get_user+0x9d4/0x1290 [tun]
[<000000000656b60d>] tun_chr_write_iter+0x4d/0x70 [tun]
[<00000000a0791a09>] __vfs_write+0x114/0x1a0
[<0000000043af9738>] vfs_write+0xb0/0x190
[<0000000093a5d2f3>] ksys_write+0x5a/0xd0
[<00000000ca8283f7>] do_syscall_64+0x55/0x100
[<00000000ea7ed8f5>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0000000005a897b6>] 0xffffffffffffffff
unreferenced object 0xffff98372c4c7800 (size 232):
comm "softirq", pid 0, jiffies 4296768010 (age 1189.920s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 10 f7 54 37 98 ff ff 00 00 00 00 00 00 00 00 ...T7...........
backtrace:
[<00000000857c2a4c>] build_skb+0x11/0x80
[<0000000076f6d169>] tun_build_skb.isra.56+0x191/0x4d0 [tun]
[<00000000c88dc3b6>] tun_get_user+0x9d4/0x1290 [tun]
[<000000000656b60d>] tun_chr_write_iter+0x4d/0x70 [tun]
[<00000000a0791a09>] __vfs_write+0x114/0x1a0
[<0000000043af9738>] vfs_write+0xb0/0x190
[<0000000093a5d2f3>] ksys_write+0x5a/0xd0
[<00000000ca8283f7>] do_syscall_64+0x55/0x100
[<00000000ea7ed8f5>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0000000005a897b6>] 0xffffffffffffffff
^ permalink raw reply
* Re: [iproute2] Can't create ip6 tunnel device
From: Andrea Claudi @ 2019-07-02 13:52 UTC (permalink / raw)
To: Ji Jianwen; +Cc: netdev, Stephen Hemminger, Mahesh Bandewar
In-Reply-To: <CAGWhr0CmF1Cz0cFE82k=vXCv7-=5Rxd97JcEn173ufU-UbQtxg@mail.gmail.com>
On Tue, Jul 2, 2019 at 3:11 PM Ji Jianwen <jijianwen@gmail.com> wrote:
>
> It works for 'add', but not for 'del'.
> ip -6 tunnel del my_ip6ip6 mode ip6ip6 remote 2001:db8:ffff:100::2
> local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev eno1
> delete tunnel "eno1" failed: Operation not supported
>
Thanks Jianwen, this is kinda expected, since I left out the
SIOCDELTUNNEL case in my code.
While this can be easily fixed, the intent of the offending patch is
not entirely clear to me.
From the ip tunnel man page, I can read that with "dev NAME" we
instruct ip to bind the tunnel to the device NAME; so dev should not
be used to indicate the tunnel, as the offending commit does.
Moreover, man page states that "ip tunnel show" has no arguments. So,
either we update the man page fixing this obsolete statement, or the
"show dev NAME" case is not supported at all.
However, even if "show" command supports filter (as it seems to do),
in my opinion "dev NAME" should be used to filter tunnels based on the
device to which they are binded.
Mahesh, can you please clarify?
Regards,
Andrea
> On Tue, Jul 2, 2019 at 7:18 PM Andrea Claudi <aclaudi@redhat.com> wrote:
> >
> > On Tue, Jul 2, 2019 at 12:55 PM Andrea Claudi <aclaudi@redhat.com> wrote:
> > >
> > > On Tue, Jul 2, 2019 at 12:27 PM Ji Jianwen <jijianwen@gmail.com> wrote:
> > > >
> > > > It seems this issue was introduced by commit below, I am able to run
> > > > the command successfully mentioned at previous mail without it.
> > > >
> > > > commit ba126dcad20e6d0e472586541d78bdd1ac4f1123 (HEAD)
> > > > Author: Mahesh Bandewar <maheshb@google.com>
> > > > Date: Thu Jun 6 16:44:26 2019 -0700
> > > >
> > > > ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds
> > > >
> > >
> > > From what I can see, before this commit we have in p->name the tunnel
> > > iface name (in Jianwen example, ip6tnl1), while after this p->name
> > > contains the iface name specified after "dev".
> > > Probably the strlcpy() should be limited to the {show|change} cases?
> > >
> > > Regards,
> > > Andrea
> > >
> > > > On Tue, Jul 2, 2019 at 2:53 PM Ji Jianwen <jijianwen@gmail.com> wrote:
> > > > >
> > > > > Hello there,
> > > > >
> > > > > I got error when creating ip6 tunnel device on a rhel-8.0.0 system.
> > > > >
> > > > > Here are the steps to reproduce the issue.
> > > > > # # uname -r
> > > > > 4.18.0-80.el8.x86_64
> > > > > # dnf install -y libcap-devel bison flex git gcc
> > > > > # git clone git://git.kernel.org/pub/scm/network/iproute2/iproute2.git
> > > > > # cd iproute2 && git log --pretty=oneline --abbrev-commit
> > > > > d0272f54 (HEAD -> master, origin/master, origin/HEAD) devlink: fix
> > > > > libc and kernel headers collision
> > > > > ee09370a devlink: fix format string warning for 32bit targets
> > > > > 68c46872 ip address: do not set mngtmpaddr option for IPv4 addresses
> > > > > e4448b6c ip address: do not set home option for IPv4 addresses
> > > > > ....
> > > > >
> > > > > # ./configure && make && make install
> > > > > # ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2
> > > > > local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev eno1 --->
> > > > > please replace eno1 with the network card name of your system
> > > > > add tunnel "ip6tnl0" failed: File exists
> > > > >
> > > > > Please help take a look. Thanks!
> > > > >
> > > > > Br,
> > > > > Jianwen
> >
> > Jianwen, can you please check if this patch solves your issue?
> >
> > --- a/ip/ip6tunnel.c
> > +++ b/ip/ip6tunnel.c
> > @@ -298,7 +298,7 @@ static int parse_args(int argc, char **argv, int
> > cmd, struct ip6_tnl_parm2 *p)
> > p->link = ll_name_to_index(medium);
> > if (!p->link)
> > return nodev(medium);
> > - else
> > + else if (cmd != SIOCADDTUNNEL)
> > strlcpy(p->name, medium, sizeof(p->name));
> > }
> > return 0;
> >
> > Thanks in advance!
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/6] xsk: add support for need_wakeup flag in AF_XDP rings
From: Maxim Mikityanskiy @ 2019-07-02 13:46 UTC (permalink / raw)
To: Magnus Karlsson, ast@kernel.org
Cc: bjorn.topel@intel.com, daniel@iogearbox.net,
netdev@vger.kernel.org, brouer@redhat.com, bpf@vger.kernel.org,
bruce.richardson@intel.com, ciara.loftus@intel.com,
jakub.kicinski@netronome.com, xiaolong.ye@intel.com,
qi.z.zhang@intel.com, sridhar.samudrala@intel.com,
kevin.laatz@intel.com, ilias.apalodimas@linaro.org,
kiran.patil@intel.com, axboe@kernel.dk,
maciej.fijalkowski@intel.com, maciejromanfijalkowski@gmail.com,
intel-wired-lan@lists.osuosl.org
In-Reply-To: <1562059288-26773-3-git-send-email-magnus.karlsson@intel.com>
On 2019-07-02 12:21, Magnus Karlsson wrote:
>
> +/* XDP_RING flags */
> +#define XDP_RING_NEED_WAKEUP (1 << 0)
> +
> struct xdp_ring_offset {
> __u64 producer;
> __u64 consumer;
> __u64 desc;
> + __u64 flags;
> };
>
> struct xdp_mmap_offsets {
<snip>
> @@ -621,9 +692,12 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> case XDP_MMAP_OFFSETS:
> {
> struct xdp_mmap_offsets off;
> + bool flags_supported = true;
>
> - if (len < sizeof(off))
> + if (len < sizeof(off) - sizeof(off.rx.flags))
> return -EINVAL;
> + else if (len < sizeof(off))
> + flags_supported = false;
>
> off.rx.producer = offsetof(struct xdp_rxtx_ring, ptrs.producer);
> off.rx.consumer = offsetof(struct xdp_rxtx_ring, ptrs.consumer);
> @@ -638,6 +712,16 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> off.cr.producer = offsetof(struct xdp_umem_ring, ptrs.producer);
> off.cr.consumer = offsetof(struct xdp_umem_ring, ptrs.consumer);
> off.cr.desc = offsetof(struct xdp_umem_ring, desc);
> + if (flags_supported) {
> + off.rx.flags = offsetof(struct xdp_rxtx_ring,
> + ptrs.flags);
> + off.tx.flags = offsetof(struct xdp_rxtx_ring,
> + ptrs.flags);
> + off.fr.flags = offsetof(struct xdp_umem_ring,
> + ptrs.flags);
> + off.cr.flags = offsetof(struct xdp_umem_ring,
> + ptrs.flags);
> + }
As far as I understood (correct me if I'm wrong), you are trying to
preserve backward compatibility, so that if userspace doesn't support
the flags field, you will determine that by looking at len and fall back
to the old format.
However, two things are broken here:
1. The check `len < sizeof(off) - sizeof(off.rx.flags)` should be `len <
sizeof(off) - 4 * sizeof(flags)`, because struct xdp_mmap_offsets
consists of 4 structs xdp_ring_offset.
2. The old and new formats are not binary compatible, as flags are
inserted in the middle of struct xdp_mmap_offsets.
^ permalink raw reply
* Re: [PATCH v5 net-next 6/6] net: ethernet: ti: cpsw: add XDP support
From: Jesper Dangaard Brouer @ 2019-07-02 13:39 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, davem, ast, linux-kernel, linux-omap,
ilias.apalodimas, netdev, daniel, jakub.kicinski, john.fastabend,
brouer
In-Reply-To: <20190702113738.GB4510@khorivan>
On Tue, 2 Jul 2019 14:37:39 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Mon, Jul 01, 2019 at 06:19:01PM +0200, Jesper Dangaard Brouer wrote:
> >On Sun, 30 Jun 2019 20:23:48 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> +static int cpsw_ndev_create_xdp_rxq(struct cpsw_priv *priv, int ch)
> >> +{
> >> + struct cpsw_common *cpsw = priv->cpsw;
> >> + int ret, new_pool = false;
> >> + struct xdp_rxq_info *rxq;
> >> +
> >> + rxq = &priv->xdp_rxq[ch];
> >> +
> >> + ret = xdp_rxq_info_reg(rxq, priv->ndev, ch);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!cpsw->page_pool[ch]) {
> >> + ret = cpsw_create_rx_pool(cpsw, ch);
> >> + if (ret)
> >> + goto err_rxq;
> >> +
> >> + new_pool = true;
> >> + }
> >> +
> >> + ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL,
> >> + cpsw->page_pool[ch]);
> >> + if (!ret)
> >> + return 0;
> >> +
> >> + if (new_pool) {
> >> + page_pool_free(cpsw->page_pool[ch]);
> >> + cpsw->page_pool[ch] = NULL;
> >> + }
> >> +
> >> +err_rxq:
> >> + xdp_rxq_info_unreg(rxq);
> >> + return ret;
> >> +}
> >
> >Looking at this, and Ilias'es XDP-netsec error handling path, it might
> >be a mistake that I removed page_pool_destroy() and instead put the
> >responsibility on xdp_rxq_info_unreg().
>
> As for me this is started not from page_pool_free, but rather from calling
> unreg_mem_model from rxq_info_unreg. Then, if page_pool_free is hidden
> it looks more a while normal to move all chain to be self destroyed.
>
> >
> >As here, we have to detect if page_pool_create() was a success, and then
> >if xdp_rxq_info_reg_mem_model() was a failure, explicitly call
> >page_pool_free() because the xdp_rxq_info_unreg() call cannot "free"
> >the page_pool object given it was not registered.
>
> Yes, it looked a little bit ugly from the beginning, but, frankly,
> I have got used to this already.
>
> >
> >Ivan's patch in[1], might be a better approach, which forced all
> >drivers to explicitly call page_pool_free(), even-though it just
> >dec-refcnt and the real call to page_pool_free() happened via
> >xdp_rxq_info_unreg().
> >
> >To better handle error path, I would re-introduce page_pool_destroy(),
>
> So, you might to do it later as I understand, and not for my special
> case but becouse it makes error path to look a little bit more pretty.
> I'm perfectly fine with this, and better you add this, for now my
> implementation requires only "xdp: allow same allocator usage" patch,
> but if you insist I can resend also patch in question afterwards my
> series is applied (with modification to cpsw & netsec & mlx5 & page_pool).
>
> What's your choice? I can add to your series patch needed for cpsw to
> avoid some misuse.
I will try to create a cleaned-up version of your patch[1] and
re-introduce page_pool_destroy() for drivers to use, then we can build
your driver on top of that.
> >as a driver API, that would gracefully handle NULL-pointer case, and
> >then call page_pool_free() with the atomic_dec_and_test(). (It should
> >hopefully simplify the error handling code a bit)
> >
> >[1] https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/
[...]
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: kernel BUG at net/rxrpc/local_object.c:LINE!
From: David Howells @ 2019-07-02 13:37 UTC (permalink / raw)
To: syzbot, ebiggers
Cc: dhowells, davem, linux-afs, linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <0000000000004c2416058c594b30@google.com>
syzbot <syzbot+1e0edc4b8b7494c28450@syzkaller.appspotmail.com> wrote:
I *think* the reproducer boils down to the attached, but I can't get syzkaller
to work and the attached sample does not cause the oops to occur. Can you try
it in your environment?
> The bug was bisected to:
>
> commit 46894a13599a977ac35411b536fb3e0b2feefa95
> Author: David Howells <dhowells@redhat.com>
> Date: Thu Oct 4 08:32:28 2018 +0000
>
> rxrpc: Use IPv4 addresses throught the IPv6
This might not be the correct bisection point. If you look at the attached
sample, you're mixing AF_INET and AF_INET6. If you try AF_INET throughout,
that might get a different point. On the other hand, since you've bound the
socket, the AF_INET6 passed to socket() should be ignored.
David
---
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <linux/rxrpc.h>
static const unsigned char inet4_addr[4] = {
0xe0, 0x00, 0x00, 0x01
};
int main(void)
{
struct sockaddr_rxrpc srx;
int fd;
memset(&srx, 0, sizeof(srx));
srx.srx_family = AF_RXRPC;
srx.srx_service = 0;
srx.transport_type = AF_INET;
srx.transport_len = sizeof(srx.transport.sin);
srx.transport.sin.sin_family = AF_INET;
srx.transport.sin.sin_port = htons(0x4e21);
memcpy(&srx.transport.sin.sin_addr, inet4_addr, 4);
fd = socket(AF_RXRPC, SOCK_DGRAM, AF_INET6);
if (fd == -1) {
perror("socket");
exit(1);
}
if (bind(fd, (struct sockaddr *)&srx, sizeof(srx)) == -1) {
perror("bind");
exit(1);
}
sleep(20);
// Whilst sleeping, hit with:
// echo -e '\0\0\0\0\0\0\0\0' | ncat -4u --send-only 224.0.0.1 20001
return 0;
}
^ permalink raw reply
* [PATCH bpf-next] bpf: cgroup: Fix build error without CONFIG_NET
From: YueHaibing @ 2019-07-02 13:29 UTC (permalink / raw)
To: ast, daniel, kafai, songliubraving, yhs, sdf
Cc: linux-kernel, netdev, bpf, YueHaibing
If CONFIG_NET is not set, gcc building fails:
kernel/bpf/cgroup.o: In function `cg_sockopt_func_proto':
cgroup.c:(.text+0x237e): undefined reference to `bpf_sk_storage_get_proto'
cgroup.c:(.text+0x2394): undefined reference to `bpf_sk_storage_delete_proto'
kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_getsockopt':
(.text+0x2a1f): undefined reference to `lock_sock_nested'
(.text+0x2ca2): undefined reference to `release_sock'
kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_setsockopt':
(.text+0x3006): undefined reference to `lock_sock_nested'
(.text+0x32bb): undefined reference to `release_sock'
Add CONFIG_NET dependency to fix this.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
init/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/init/Kconfig b/init/Kconfig
index e2e51b5..341cf2a 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -998,6 +998,7 @@ config CGROUP_PERF
config CGROUP_BPF
bool "Support for eBPF programs attached to cgroups"
depends on BPF_SYSCALL
+ depends on NET
select SOCK_CGROUP_DATA
help
Allow attaching eBPF programs to a cgroup using the bpf(2)
--
2.7.4
^ permalink raw reply related
* Re: kernel panic: corrupted stack end in dput
From: Al Viro @ 2019-07-02 13:21 UTC (permalink / raw)
To: Hillf Danton; +Cc: syzbot, linux-fsdevel, linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <000000000000a5d3cb058c9a64f0@google.com>
On Tue, Jul 02, 2019 at 05:21:26PM +0800, Hillf Danton wrote:
> Hello,
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -673,14 +673,11 @@ static struct dentry *dentry_kill(struct dentry *dentry)
> if (!IS_ROOT(dentry)) {
> parent = dentry->d_parent;
> if (unlikely(!spin_trylock(&parent->d_lock))) {
> - parent = __lock_parent(dentry);
> - if (likely(inode || !dentry->d_inode))
> - goto got_locks;
> - /* negative that became positive */
> - if (parent)
> - spin_unlock(&parent->d_lock);
> - inode = dentry->d_inode;
> - goto slow_positive;
> + /*
> + * fine if peer is busy either populating or
> + * cleaning up parent
> + */
> + parent = NULL;
> }
> }
> __dentry_kill(dentry);
This is very much *NOT* fine.
1) trylock can fail from any number of reasons, starting
with "somebody is going through the hash chain doing a lookup on
something completely unrelated"
2) whoever had been holding the lock and whatever they'd
been doing might be over right after we get the return value from
spin_trylock().
3) even had that been really somebody adding children in
the same parent *AND* even if they really kept doing that, rather
than unlocking and buggering off, would you care to explain why
dentry_unlist() called by __dentry_kill() and removing the victim
from the list of children would be safe to do in parallel with that?
NAK, in case it's not obvious from the above.
^ permalink raw reply
* [PATCH] nfc: st-nci: remove redundant assignment to variable r
From: Colin King @ 2019-07-02 13:16 UTC (permalink / raw)
To: Thomas Gleixner, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The variable r is being initialized with a value that is never
read and it is being updated later with a new value. The
initialization is redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/nfc/st-nci/i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
index 67a685adfd44..55d600cd3861 100644
--- a/drivers/nfc/st-nci/i2c.c
+++ b/drivers/nfc/st-nci/i2c.c
@@ -72,7 +72,7 @@ static void st_nci_i2c_disable(void *phy_id)
*/
static int st_nci_i2c_write(void *phy_id, struct sk_buff *skb)
{
- int r = -1;
+ int r;
struct st_nci_i2c_phy *phy = phy_id;
struct i2c_client *client = phy->i2c_dev;
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox