* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Alex Elder @ 2019-06-24 16:21 UTC (permalink / raw)
To: Johannes Berg, Arnd Bergmann, Dan Williams
Cc: Subash Abhinov Kasiviswanathan, abhishek.esse, Ben Chan,
Bjorn Andersson, cpratapa, David Miller, DTML, Eric Caruso,
evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <abdfc6b3a9981bcdef40f85f5442a425ce109010.camel@sipsolutions.net>
On 6/18/19 2:03 PM, Johannes Berg wrote:
> On Tue, 2019-06-18 at 08:45 -0500, Alex Elder wrote:
>
>> If it had a well-defined way of creating new channels to be
>> multiplexed over the connection to the modem, the IPA driver
>> (rather than the rmnet driver) could present network interfaces
>> for each and perform the multiplexing.
>
> Right. That's what I was thinking of.
. . .
>> But I think the IPA driver would register with the WWAN core as
>> a "provider," and then the WWAN core would subsequently request
>> that it instantiate netdevices to represent channels on demand
>> (rather than registering them).
>
> Yeah, I guess you could call it that way.
>
> Really there are two possible ways (and they intersect to some extent).
>
> One is the whole multi-function device, where a single WWAN device is
> composed of channels offered by actually different drivers, e.g. for a
> typical USB device you might have something like cdc_ether and the
> usb_wwan TTY driver. In this way, we need to "compose" the WWAN device
> similarly, e.g. by using the underlying USB device "struct device"
> pointer to tie it together.
I *think* this model makes the most sense. But at this point
it would take very little to convince me otherwise... (And then
I saw Arnd's message advocating the other one, unfortunately...)
> The other is something like IPA or the Intel modem driver, where the
> device is actually a single (e.g. PCIe) device and just has a single
> driver, but that single driver offers different channels.
What I don't like about this is that it's more monolithic. It
seems better to have the low-level IPA or Intel modem driver (or
any other driver that can support communication between the AP
and WWAN device) present communication paths that other function-
specific drivers can attach to and use.
> Now, it's not clear to me where IPA actually falls, because so far we've
> been talking about the IPA driver only as providing *netdevs*, not any
> control channels, so I'm not actually sure where the control channel is.
There is user space code that handles all of this, and as far as I
can tell, parts of it will always remain proprietary.
> For the Intel device, however, the control channel is definitely
> provided by exactly the same driver as the data channels (netdevs).
I do see the need for a control interface. But I suspect it
would *overlap* with what you describe and might need to be more
general and/or extensible. Are there control channels specific to
use for a modem--like a "modem control interface" or something?
Is there something broader, like "this WWAN device supports
functions A, and B with protocols X, Y; please open a connection
to A with protocol X." Do both exist? I'm just trying to contain
whatever a "control channel" really represents, and what it would
be associated with.
> "provider" is a good word, and in fact the Intel driver would also be a
> provider for a GNSS channel (TBD how to represent, a tty?), one or
> multiple debug/tracing channels, data channels (netdevs), AT command
> channels (mbim, ...?) (again tbd how to represent, ttys?), etc.
Yes, this is much clearer to me now.
> What I showed in the header files I posted so far was the provider only
> having "data channel" ops (create/remove a netdev) but for each channel
> type we either want a new method there, or we just change the method to
> be something like
>
> int (*create_channel)(..., enum wwan_chan_type chan_type, ...);
>
> and simply require that the channel is attached to the wwan device with
> the representation-specific call (wwan_attach_netdev, wwan_attach_tty,
> ...).
Or maybe have the WWAN device present interfaces with attributes,
and have drivers that are appropriate for each interface attach
to only the ones they recognize they support.
-Alex
> This is a bit less comfortable because then it's difficult to know what
> was actually created upon the request, so it's probably better to have
> different methods for the different types of representations (like I had
> - add_netdev, add_tty, ...).
>
> Note also that I said "representation-specific", while passing a
> "channel type", so for this we'd actually need a convention on what
> channel type has what kind of representation, which again gets awkward.
> Better to make it explicit.
>
> (And even then, we might be able to let userspace have some control,
> e.g. the driver might be able to create a debug channel as both a TTY or
> something else)
>
> johannes
>
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Alex Elder @ 2019-06-24 16:21 UTC (permalink / raw)
To: Johannes Berg, Arnd Bergmann, Dan Williams
Cc: Subash Abhinov Kasiviswanathan, abhishek.esse, Ben Chan,
Bjorn Andersson, cpratapa, David Miller, DTML, Eric Caruso,
evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <967604dd8d466a99b865649174f8b9cd34b2560e.camel@sipsolutions.net>
On 6/18/19 1:48 PM, Johannes Berg wrote:
> Just to add to Dan's response, I think he's captured our discussions and
> thoughts well.
>
>> First, a few terms (correct or improve as you like):
>
> Thanks for defining, we don't do that nearly often enough.
>
>> - WWAN device is a hardware device (like IPA) that presents a
>> connection between AP and modem, and presents an interface
>> that allows the use of that connection to be managed.
>
> Yes. But I was actually thinking of a "wwan_dev" to be a separate
> structure, not *directly* owned by a single driver and used to represent
> the hardware like a (hypothetical) "struct ipa_dev".
I think you're talking about creating a coordination interface
that allows multiple drivers to interact with a WWAN device,
which might implement several independent features.
>> - WWAN netdevice represents a Linux network interface, with its
>> operations and queues, etc., but implements a standardized
>> set of WWAN-specific operations. It represents a logical
>> ' channel whose data is multiplexed over the WWAN device.
>
> I'm not sure I'd asy it has much WWAN-specific operations? But yeah, I
> guess it might.
I want to withdraw this notion of a "WWAN netdevice"...
>> - WWAN channel is a user space abstraction that corresponds
>> with a WWAN netdevice (but I'm not clear on all the ways
>> they differ or interact).
>
> As Dan said, this could be a different abstraction than a netdevice,
> like a TTY, etc.
Right, I get that now.
. . .
>> - Which WWAN channel attributes must be set *before* the
>> channel is activated, and can't be changed? Are there any
>> that can be changed dynamically?
>
> It's a good question. I threw a "u32 pdn" in there, but I'm not actually
> sure that's what you *really* need?
>
> Maybe the modem and userspace just agree on some arbitrary "session
> identifier"? Dan mentions "MUX ID" or "MBIM Session ID", maybe there
> really is no good general term for this and we should just call it a
> "session identifier" and agree that it depends on the control protocol
> (MBIM vs. QMI vs. ...)?
>
>> And while the whole point of this is to make things generic,
>> it might be nice to have a way to implement a new feature
>> before it can be "standardized".
>
> Not sure I understand this?
I'm talking about a way to experiment with new functionality in a
way that's explicitly not part of the interface. But doing that
isn't necessary and it's probably not a good idea anyway.
> FWIW, I actually came to this because we want to upstream a driver for
> an Intel modem, but ... can't really make up our mind on whether or not
> to use VLAN tags, something like rmnet (but we obviously cannot use
> rmnet, so that'd be another vendor specific interface like rmnet), or
> sysfs, or any of the other methods we have today ... :-)
OK cool then we have some common needs. Let's get this defined so
we can use it for both!
-Alex
>
> johannes
>
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Alex Elder @ 2019-06-24 16:21 UTC (permalink / raw)
To: Dan Williams, Johannes Berg, Arnd Bergmann
Cc: Subash Abhinov Kasiviswanathan, abhishek.esse, Ben Chan,
Bjorn Andersson, cpratapa, David Miller, DTML, Eric Caruso,
evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <84153d9e7c903084b492ceccc0dd98cbb32c12ac.camel@redhat.com>
On 6/18/19 1:06 PM, Dan Williams wrote:
> On Tue, 2019-06-18 at 10:20 -0500, Alex Elder wrote:
>> On 6/17/19 7:25 AM, Johannes Berg wrote:
>>> On Mon, 2019-06-17 at 13:42 +0200, Johannes Berg wrote:
>>>
>>>> But anyway, as I alluded to above, I had something like this in
>>>> mind:
Sorry for the delay. There's a lot here to go through, and with
each message the picture is (slowly) getting a bit clearer for me.
Still, there are some broad tradeoffs to consider and I think we
need to get a little more specific again. I'm going to start a
new thread (or rather re-subject a response to the very first one)
that tries to do a fresh start that takes into account the
discussion so far.
I will also be talking with some people inside Qualcomm (including
Subash) soon to make sure we don't miss any requirements or insights
they know of that I don't realize are important.
But before I send anything new I'm going to respond to a few things.
>>> I forgot to state this here, but this was *heavily* influenced by
>>> discussions with Dan - many thanks to him.
>>
>> Thanks for getting even more concrete with this. Code is the
>> most concise way of describing things, once the general ideas
>> seem to be coming together.
>>
>> I'm not going to comment on the specific code bits, but I have
>> some more general questions and comments on the design. Some
>> of these are simply due to my lack of knowledge of how WWAN/modem
>> interactions normally work.
>>
>> First, a few terms (correct or improve as you like):
>> - WWAN device is a hardware device (like IPA) that presents a
>> connection between AP and modem, and presents an interface
>> that allows the use of that connection to be managed.
>> - WWAN netdevice represents a Linux network interface, with its
>> operations and queues, etc., but implements a standardized
>> set of WWAN-specific operations. It represents a logical
>> ' channel whose data is multiplexed over the WWAN device.
>> - WWAN channel is a user space abstraction that corresponds
>> with a WWAN netdevice (but I'm not clear on all the ways
>> they differ or interact).
>
> When Johannes and I have talked about "WWAN channel" we mean a control
> or data or other channel. That could be QMI, AT, MBIM control, GPS,
> PCSC, QMAP, MBIM data, PPP TTY, DM/DIAG, CDC-ETHER, CDC-NCM, Sierra
> HIP, etc. Or even voice-call audio :)
>
> A netdev is a Linux abstraction of a WWAN *data* channel, be that QMI
> or CDC-ETHER or whatever.
I think I now understand this. My only focus with the IPA driver
has been the network data driver. I'll go into more detail later
but I now see that there are other entities on a WWAN device that
do not require a netdev.
>> - The WWAN core is kernel code that presents abstractions
>> for WWAN devices and netdevices, so they can be managed
>> in a generic way. It is for configuration and communication
>> and is not at all involved in the data path.
>>
>> You're saying that the WWAN driver space calls wwan_add()
>> to register itself as a new WWAN device.
>>
>> You're also saying that a WWAN device "attaches" a WWAN
>> netdevice, which is basically notifying the WWAN core
>> that the new netdev/channel is available for use.
>> - I trust that a "tentative" attachement is necessary. But
>> I'm not sure what makes it transition into becoming a
>> "real" one, or how that event gets communicated.
>
> Linux usually tries to keep drivers generic and focused; each driver is
> written for a specific function. For example, a USB device usually
> provides multiple USB interfaces which will be bound to different Linux
> drivers like a TTY, cdc-ether, QMI (via qmi_wwan), cdc-acm, etc.
So USB has some attributes similar to what we're talking about
here. But if I'm not mistaken we want some sort of an overall
management scheme as well.
> These drivers are often generic and we may not have enough information
> in one driver to know that the parent of this interface is a WWAN
> device. But another driver might. Since probing is asynchronous we may
> have cdc-acm bind to a device and provide a TTY before cdc-ether (which
> does know it's a WWAN) binds and provides the netdevice.
Is this why Johannes wanted to have a "maybe attach" method?
I don't like the "maybe" API unless there's no other way to do it.
Instead I think it would be better for the probing driver to register
with a whatever the WWAN core is, and then have the WWAN core be
responsible for pulling things all together when it receives a
request to do so. I.e., something in user space should request
that a registered data interface be brought up, and at that
time everything "knows" it's implemented as part of a WWAN
device.
>> Some questions:
>> - What causes a new channel to be created? Is it initiated
>> by the WWAN device driver? Does the modem request that
>> it get created? User space? Both?
>
> Either created at driver bind time in the kernel (usually control
> channels) or initiated by userspace when the WWAN management process
> has coordinated with the firmware for another channel. Honestly
So maybe:
- Hardware probe detects a WWAN device
- The drivers that detect the WWAN device register it with the
WWAN core code.
- A control channel is instantiated at/before the time the WWAN
device is registered
- Something in user space should manage the bring-up of any
other things on the WWAN device thereafter
> userspace should probably always create the netdevices (since they are
> always useless until userspace coordinates with the firmware about
> them) but that's not how things are yet.
That's too bad. How hard would that be to change?
> [ A concrete example...
>
> Assume a QMI device has an existing packet data connection which is
> abstracted by a netdevice on the Linux side. Now the WWAN management
> daemon wants to create a second packet data connection with a different
> APN (maybe an MMS connection, maybe a VOIP one, maybe an IPv6). It
> sends a WDS Start Network request to the modem firmware and receives a
> new QMI Packet Data Handle.
>
> The management daemon must somehow get a netdevice associated with this
> new Packet Data Handle. It would ask the WWAN kernel device to create a
> new data channel with the PDH, and would get back the ifindex of that
> netdevice which it would configure with the IP that it gets from the
> firmware via the WDS Get Current Settings QMI request.
>
> The WWAN device would forward the request down to IPA (or rmnet) which
> would then create the netdevice using the PDH as the QMAP MUX ID for
> that netdevice's traffic.]
OK yes I'm following this now. I appreciate the example.
>> - What causes a created channel to be removed?
>
> Driver removal, userspace WWAN daemon terminating the packet data
> connection which the channel represents, the modem terminating the
> packet data connection (eg network initiated disconnect), etc.
OK this is as I expected. Driver (or device) removal is somewhat
obvious, but you're confirming user space might request it as well.
>> - You distinguish between attaching a netdevice and (what
>> I'll call) activating it. What causes activation?
>
> Can you describe what you mean by "activating"? Do you mean
> successfully TX/RX packets via the netdev and the outside world?
Johannes mentioned an API to "maybe attach" a device. That begs
the question of what happens if this request does *not* attach.
Does the attach request have to be made again, or is it done
automatically with a notification, or something else?
So by "activation" I was trying to refer to the notion of this
subsequent successful attach.
> I read "attach" here as simply associating an existing netdev with the
> "parent" WWAN device. A purely Linux operation that is only book-
> keeping and may not have any interaction with the modem.
If that's the case I would want the "activation" to be a separate
step. The attach would do the bookkeeping, and generally shouldn't
fail. An attached interface would be brought up ("activated")
separately and might fail if things aren't quite ready yet.
>> - How are the attributes of a WWAN device or channel set,
>> or communicated?
>
> Via netlink attributes when userspace asks the WWAN device to create a
> new channel. In the control methods I've seen, only userspace really
> knows the channel identifier that it and the modem have agreed on (eg
> what the MUX ID in the QMAP header would be, or the MBIM Session ID).
Yes, that's the way it's worked for rmnet and IPA. Previously it
was IOCTL requests but it's currently hard-wired.
>> - Are there any attributes that are only optionally supported,
>> and if so, how are the supported ones communicated?
>
> Yeah, capabilities would be important here and I don't think Johannes
> accounted for that yet.
>
>> - Which WWAN channel attributes must be set *before* the
>> channel is activated, and can't be changed? Are there any
>> that can be changed dynamically?
>
> I would assume userspace must pass the agreed identifier (QMUX ID, MBIM
> session ID, etc) when creating the channel and that wouldn't change. I
> think a world where you can dynamically change the MUX ID/SessionID/etc
> is a more complicated one.
>
> Things like QoS could change but I don't recall if modems allow that;
> eg does a +CGEQOS (or equivalent QMI WDS LTE QoS Parameters request)
> take effect while the bearer is active, or is it only respected on
> bearer creation?
You are speaking in a language I'm only now coming to understand.
I think the point of my question is clear though--I think both
static and dynamic attributes need to be taken into account.
>> And while the whole point of this is to make things generic,
>> it might be nice to have a way to implement a new feature
>> before it can be "standardized".
>
> That would be nice, but I'd rather have the conversation about if/how
> to standardize things before they make it into the kernel and have
> their API set in stone... which is how we ended up with 5 ways of doing
> the same thing already.
Agreed.
-Alex
> Dan
>
>> Thanks.
>>
>> -Alex
>>
>> PS I don't want to exclude anybody but we could probably start
>> a different mail chain on this topic...
>>
>>>> driver_dev
>>>> struct device *dev (USB, PCI, ...)
>>>> net_device NA
>>>> net_device NB
>>>> tty TA
>>>> ...
>>>>
>>
>> . . .
>
^ permalink raw reply
* Re: [PATCH net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable
From: David Miller @ 2019-06-24 16:21 UTC (permalink / raw)
To: lucien.xin
Cc: eric.dumazet, netdev, jon.maloy, ying.xue, tipc-discussion,
syzkaller-bugs
In-Reply-To: <CADvbK_dSghWbMtmpH+oMpW=0CsSU-usjQ=_nZw2qkgQ0iEuH+A@mail.gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Tue, 25 Jun 2019 00:00:39 +0800
> Sorry, David, do I need to resend this one?
Yes, please, that helps me a lot.
^ permalink raw reply
* Re: [PATCH net-next] net: macb: use GRO
From: David Miller @ 2019-06-24 16:20 UTC (permalink / raw)
To: antoine.tenart
Cc: nicolas.ferre, netdev, linux-kernel, ludovic.desroches,
alexandre.belloni
In-Reply-To: <20190621153002.30587-1-antoine.tenart@bootlin.com>
From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Fri, 21 Jun 2019 17:30:02 +0200
> This patch updates the macb driver to use NAPI GRO helpers when
> receiving SKBs. This improves performances.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: macb: use NAPI_POLL_WEIGHT
From: David Miller @ 2019-06-24 16:20 UTC (permalink / raw)
To: antoine.tenart
Cc: nicolas.ferre, netdev, linux-kernel, ludovic.desroches,
alexandre.belloni
In-Reply-To: <20190621152855.30330-1-antoine.tenart@bootlin.com>
From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Fri, 21 Jun 2019 17:28:55 +0200
> Use NAPI_POLL_WEIGHT, the default NAPI poll() weight instead of
> redefining our own value (which turns out to be 64 as well).
>
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH rdma-next v1 10/12] IB/mlx5: Enable subscription for device events over DEVX
From: Yishai Hadas @ 2019-06-24 16:13 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20190624115726.GC5479@mellanox.com>
On 6/24/2019 2:57 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2019 at 08:15:38PM +0300, Leon Romanovsky wrote:
>> From: Yishai Hadas <yishaih@mellanox.com>
>>
>> Enable subscription for device events over DEVX.
>>
>> Each subscription is added to the two level XA data structure according
>> to its event number and the DEVX object information in case was given
>> with the given target fd.
>>
>> Those events will be reported over the given fd once will occur.
>> Downstream patches will mange the dispatching to any subscription.
>>
>> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>> drivers/infiniband/hw/mlx5/devx.c | 564 ++++++++++++++++++++++-
>> include/uapi/rdma/mlx5_user_ioctl_cmds.h | 9 +
>> 2 files changed, 566 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
>> index e9b9ba5a3e9a..304b13e7a265 100644
>> +++ b/drivers/infiniband/hw/mlx5/devx.c
>> @@ -14,6 +14,7 @@
>> #include <linux/mlx5/driver.h>
>> #include <linux/mlx5/fs.h>
>> #include "mlx5_ib.h"
>> +#include <linux/xarray.h>
>>
>> #define UVERBS_MODULE_NAME mlx5_ib
>> #include <rdma/uverbs_named_ioctl.h>
>> @@ -33,6 +34,37 @@ struct devx_async_data {
>> struct mlx5_ib_uapi_devx_async_cmd_hdr hdr;
>> };
>>
>> +/* first level XA value data structure */
>> +struct devx_event {
>> + struct xarray object_ids; /* second XA level, Key = object id */
>> + struct list_head unaffiliated_list;
>> +};
>> +
>> +/* second level XA value data structure */
>> +struct devx_obj_event {
>> + struct rcu_head rcu;
>> + struct list_head obj_sub_list;
>> +};
>> +
>> +struct devx_event_subscription {
>> + struct list_head file_list; /* headed in private_data->
>> + * subscribed_events_list
>> + */
>> + struct list_head xa_list; /* headed in devx_event->unaffiliated_list or
>> + * devx_obj_event->obj_sub_list
>> + */
>> + struct list_head obj_list; /* headed in devx_object */
>> +
>> + u32 xa_key_level1;
>> + u32 xa_key_level2;
>> + struct rcu_head rcu;
>> + u64 cookie;
>> + bool is_obj_related;
>> + struct ib_uobject *fd_uobj;
>> + void *object; /* May need direct access upon hot unplug */
>
> This should be a 'struct file *' and have a better name.
>
OK, will change.
> And I'm unclear why we need to store both the ib_uobject and the
> struct file for the same thing?
Post hot unplug/unbind the uobj can't be accessed any more to reach the
object as it will be set to NULL by ib_core layer [1].
As the filp is still open we need a direct access to it down the road
and for that we have it separately.
This was the comment that I have just put above in the code, I may
improve it with more details as pointed here.
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/rdma_core.c#L149
And why are we storing the uobj here
> instead of the struct devx_async_event_file *?
>
Basically storing the uobj is the same as of storing the
devx_async_event_file, as we just use container_of to get it from.
There is no direct access from uobj to any of its fields so that we
should be fine.
> Since uobj->object == flip && filp->private_data == uobj, I have a
> hard time to understand why we need both things, it seems to me that
> if we get the fget on the filp then we can rely on the
> filp->private_data to get back to the devx_async_event_file.
>
The idea was to not take an extra ref count on the file (i.e. fget) per
subscription, this will let the release option to be called once the
file will be closed by the application.
Otherwise we might need to consider having some unsubscribe method to
put the ref count back with all its overhead and implications without a
real justified reason.
>> + struct eventfd_ctx *eventfd;
>> +};
>> +
>
>> /*
>> * As the obj_id in the firmware is not globally unique the object type
>> * must be considered upon checking for a valid object id.
>> @@ -1143,14 +1275,53 @@ static void devx_cleanup_mkey(struct devx_obj *obj)
>> write_unlock_irqrestore(&table->lock, flags);
>> }
>>
>> +static void devx_cleanup_subscription(struct mlx5_ib_dev *dev,
>> + struct devx_event_subscription *sub)
>> +{
>> + list_del_rcu(&sub->file_list);
>> + list_del_rcu(&sub->xa_list);
>> +
>> + if (sub->is_obj_related) {
>
> is_obj_related looks like it is just list_empty(obj_list)??
>
Yes, in that approach we may need to call INIT_LIST_HEAD(sub->obj_list)
in case it wasn't an object upon subscription, will do that.
> Success oriented flow
>
>> @@ -1523,6 +1700,350 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_ASYNC_QUERY)(
>> return err;
>> }
>>
>> +static void
>> +subscribe_event_xa_dealloc(struct mlx5_devx_event_table *devx_event_table,
>> + u32 key_level1,
>> + u32 key_level2,
>> + struct devx_obj_event *alloc_obj_event)
>> +{
>> + struct devx_event *event;
>> +
>> + /* Level 1 is valid for future use - no need to free */
>> + if (!alloc_obj_event)
>> + return;
>> +
>> + event = xa_load(&devx_event_table->event_xa, key_level1);
>> + WARN_ON(!event);
>> +
>> + xa_erase(&event->object_ids, key_level2);
>
> Shoulnd't this only erase if the value stored is NULL?
>
If this key_level2 wasn't allocated by the subscribe flow and exists
before we may not reach here and return from the above lines in this
function [1], otherwise we need to erase as done here.
[1]
if (!alloc_obj_event)
return;
>> + kfree(alloc_obj_event);
>> +}
>> +
>> +static int
>> +subscribe_event_xa_alloc(struct mlx5_devx_event_table *devx_event_table,
>> + u32 key_level1,
>> + bool is_level2,
>> + u32 key_level2,
>> + struct devx_obj_event **alloc_obj_event)
>> +{
>> + struct devx_obj_event *obj_event;
>> + struct devx_event *event;
>> + bool new_entry_level1 = false;
>> + int err;
>> +
>> + event = xa_load(&devx_event_table->event_xa, key_level1);
>> + if (!event) {
>> + event = kzalloc(sizeof(*event), GFP_KERNEL);
>> + if (!event)
>> + return -ENOMEM;
>> +
>> + new_entry_level1 = true;
>> + INIT_LIST_HEAD(&event->unaffiliated_list);
>> + xa_init(&event->object_ids);
>> +
>> + err = xa_insert(&devx_event_table->event_xa,
>> + key_level1,
>> + event,
>> + GFP_KERNEL);
>> + if (err)
>> + goto end;
>> + }
>> +
>> + if (!is_level2)
>> + return 0;
>> +
>> + obj_event = xa_load(&event->object_ids, key_level2);
>> + if (!obj_event) {
>> + err = xa_reserve(&event->object_ids, key_level2, GFP_KERNEL);
>> + if (err)
>> + goto err_level1;
>> +
>> + obj_event = kzalloc(sizeof(*obj_event), GFP_KERNEL);
>> + if (!obj_event) {
>> + err = -ENOMEM;
>> + goto err_level2;
>> + }
>> +
>> + INIT_LIST_HEAD(&obj_event->obj_sub_list);
>> + *alloc_obj_event = obj_event;
>
> This is goofy, just store the empty obj_event in the xa instead of
> using xa_reserve, and when you go to do the error unwind just delete
> any level2' devx_obj_event' that has a list_empty(obj_sub_list), get
> rid of the wonky alloc_obj_event stuff.
>
Please see my answer above about how level2 is managed by this
alloc_obj_event, is that really worth a change ? I found current logic
to be clear. I may put some note here if we can stay with that.
> The best configuration would be to use devx_cleanup_subscription to
> undo the partially ready subscription.
>
This partially ready subscription might not match the
devx_cleanup_subscription(), e.g. it wasn't added to xa_list and can't
be deleted without any specific flag to ignore ..
>> + }
>> +
>> + return 0;
>> +
>> +err_level2:
>> + xa_erase(&event->object_ids, key_level2);
>> +
>> +err_level1:
>> + if (new_entry_level1)
>> + xa_erase(&devx_event_table->event_xa, key_level1);
>> +end:
>> + if (new_entry_level1)
>> + kfree(event);
>
> Can't do this, once the level1 is put in the tree it could be referenced by
> the irqs. At least it needs a kfree_rcu, most likely it is simpler to
> just leave it.
>
Agree, it looks simpler just to leave it, will handle.
>> +#define MAX_NUM_EVENTS 16
>> +static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_SUBSCRIBE_EVENT)(
>> + struct uverbs_attr_bundle *attrs)
>> +{
>> + struct ib_uobject *devx_uobj = uverbs_attr_get_uobject(
>> + attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_OBJ_HANDLE);
>> + struct mlx5_ib_ucontext *c = rdma_udata_to_drv_context(
>> + &attrs->driver_udata, struct mlx5_ib_ucontext, ibucontext);
>> + struct mlx5_ib_dev *dev = to_mdev(c->ibucontext.device);
>> + struct ib_uobject *fd_uobj;
>> + struct devx_obj *obj = NULL;
>> + struct devx_async_event_file *ev_file;
>> + struct mlx5_devx_event_table *devx_event_table = &dev->devx_event_table;
>> + u16 *event_type_num_list;
>> + struct devx_event_subscription **event_sub_arr;
>> + struct devx_obj_event **event_obj_array_alloc;
>> + int redirect_fd;
>> + bool use_eventfd = false;
>> + int num_events;
>> + int num_alloc_xa_entries = 0;
>> + u16 obj_type = 0;
>> + u64 cookie = 0;
>> + u32 obj_id = 0;
>> + int err;
>> + int i;
>> +
>> + if (!c->devx_uid)
>> + return -EINVAL;
>> +
>> + if (!IS_ERR(devx_uobj)) {
>> + obj = (struct devx_obj *)devx_uobj->object;
>> + if (obj)
>> + obj_id = get_dec_obj_id(obj->obj_id);
>> + }
>> +
>> + fd_uobj = uverbs_attr_get_uobject(attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_FD_HANDLE);
>> + if (IS_ERR(fd_uobj))
>> + return PTR_ERR(fd_uobj);
>> +
>> + ev_file = container_of(fd_uobj, struct devx_async_event_file,
>> + uobj);
>> +
>> + if (uverbs_attr_is_valid(attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_FD_NUM)) {
>> + err = uverbs_copy_from(&redirect_fd, attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_FD_NUM);
>> + if (err)
>> + return err;
>> +
>> + use_eventfd = true;
>> + }
>> +
>> + if (uverbs_attr_is_valid(attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_COOKIE)) {
>> + if (use_eventfd)
>> + return -EINVAL;
>> +
>> + err = uverbs_copy_from(&cookie, attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_COOKIE);
>> + if (err)
>> + return err;
>> + }
>> +
>> + num_events = uverbs_attr_ptr_get_array_size(
>> + attrs, MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_TYPE_NUM_LIST,
>> + sizeof(u16));
>> +
>> + if (num_events < 0)
>> + return num_events;
>> +
>> + if (num_events > MAX_NUM_EVENTS)
>> + return -EINVAL;
>> +
>> + event_type_num_list = uverbs_attr_get_alloced_ptr(attrs,
>> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_TYPE_NUM_LIST);
>> +
>> + if (!is_valid_events(dev->mdev, num_events, event_type_num_list, obj))
>> + return -EINVAL;
>> +
>> + event_sub_arr = uverbs_zalloc(attrs,
>> + MAX_NUM_EVENTS * sizeof(struct devx_event_subscription *));
>> + event_obj_array_alloc = uverbs_zalloc(attrs,
>> + MAX_NUM_EVENTS * sizeof(struct devx_obj_event *));
>
> There are so many list_heads in the devx_event_subscription, why not
> use just one of them to store the allocated events instead of this
> temp array? ie event_list looks good for this purpose.
>
I'm using the array later on with direct access to the index that should
be de-allocated. I would prefer staying with this array rather than
using the 'event_list' which has other purpose down the road, it's used
per subscription and doesn't look match to hold the devx_obj_event which
has no list entry for this purpose..
>> +
>> + if (!event_sub_arr || !event_obj_array_alloc)
>> + return -ENOMEM;
>> +
>> + /* Protect from concurrent subscriptions to same XA entries to allow
>> + * both to succeed
>> + */
>> + mutex_lock(&devx_event_table->event_xa_lock);
>> + for (i = 0; i < num_events; i++) {
>> + u32 key_level1;
>> +
>> + if (obj)
>> + obj_type = get_dec_obj_type(obj,
>> + event_type_num_list[i]);
>> + key_level1 = event_type_num_list[i] | obj_type << 16;
>> +
>> + err = subscribe_event_xa_alloc(devx_event_table,
>> + key_level1,
>> + obj ? true : false,
>> + obj_id,
>> + &event_obj_array_alloc[i]);
>
> Usless ?:
What do you suggest instead ?
>
>> + if (err)
>> + goto err;
>> +
>> + num_alloc_xa_entries++;
>> + event_sub_arr[i] = kzalloc(sizeof(*event_sub_arr[i]),
>> + GFP_KERNEL);
>> + if (!event_sub_arr[i])
>> + goto err;
>> +
>> + if (use_eventfd) {
>> + event_sub_arr[i]->eventfd =
>> + eventfd_ctx_fdget(redirect_fd);
>> +
>> + if (IS_ERR(event_sub_arr[i]->eventfd)) {
>> + err = PTR_ERR(event_sub_arr[i]->eventfd);
>> + event_sub_arr[i]->eventfd = NULL;
>> + goto err;
>> + }
>> + }
>> +
>> + event_sub_arr[i]->cookie = cookie;
>> + event_sub_arr[i]->fd_uobj = fd_uobj;
>> + event_sub_arr[i]->object = fd_uobj->object;
>> + /* May be needed upon cleanup the devx object/subscription */
>> + event_sub_arr[i]->xa_key_level1 = key_level1;
>> + event_sub_arr[i]->xa_key_level2 = obj_id;
>> + event_sub_arr[i]->is_obj_related = obj ? true : false;
>
^ permalink raw reply
* Re: [PATCH v2 0/3] fix bugs when enable route_localnet
From: David Miller @ 2019-06-24 16:03 UTC (permalink / raw)
To: luoshijie1
Cc: tgraf, dsahern, netdev, liuzhiqiang26, wangxiaogang3, mingfangsen,
zhoukang7
In-Reply-To: <1560870845-172395-1-git-send-email-luoshijie1@huawei.com>
From: luoshijie <luoshijie1@huawei.com>
Date: Tue, 18 Jun 2019 15:14:02 +0000
> From: Shijie Luo <luoshijie1@huawei.com>
>
> When enable route_localnet, route of the 127/8 address is enabled.
> But in some situations like arp_announce=2, ARP requests or reply
> work abnormally.
>
> This patchset fix some bugs when enable route_localnet.
>
> Change History:
> V2:
> - Change a single patch to a patchset.
> - Add bug fix for arp_ignore = 3.
> - Add a couple of test for enabling route_localnet in selftests.
Series applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable
From: Xin Long @ 2019-06-24 16:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: network dev, davem, Jon Maloy, Ying Xue, tipc-discussion,
syzkaller-bugs
In-Reply-To: <061d3bd2-46a2-04aa-a3f7-3091e6ff8523@gmail.com>
On Mon, Jun 24, 2019 at 4:33 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 6/24/19 12:59 AM, Xin Long wrote:
> > This patch is to fix an uninit-value issue, reported by syzbot:
> >
> > BUG: KMSAN: uninit-value in memchr+0xce/0x110 lib/string.c:981
> > Call Trace:
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> > kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:622
> > __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
> > memchr+0xce/0x110 lib/string.c:981
> > string_is_valid net/tipc/netlink_compat.c:176 [inline]
> > tipc_nl_compat_bearer_disable+0x2a1/0x480 net/tipc/netlink_compat.c:449
> > __tipc_nl_compat_doit net/tipc/netlink_compat.c:327 [inline]
> > tipc_nl_compat_doit+0x3ac/0xb00 net/tipc/netlink_compat.c:360
> > tipc_nl_compat_handle net/tipc/netlink_compat.c:1178 [inline]
> > tipc_nl_compat_recv+0x1b1b/0x27b0 net/tipc/netlink_compat.c:1281
> >
> > TLV_GET_DATA_LEN() may return a negtive int value, which will be
> > used as size_t (becoming a big unsigned long) passed into memchr,
> > cause this issue.
> >
> > Similar to what it does in tipc_nl_compat_bearer_enable(), this
> > fix is to return -EINVAL when TLV_GET_DATA_LEN() is negtive in
> > tipc_nl_compat_bearer_disable(), as well as in
> > tipc_nl_compat_link_stat_dump() and tipc_nl_compat_link_reset_stats().
> >
> > Reported-by: syzbot+30eaa8bf392f7fafffaf@syzkaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> Please add an appropriate Fixes: tag, thanks !
>
Fixes: 0762216c0ad2 ("tipc: fix uninit-value in tipc_nl_compat_bearer_enable")
Fixes: 8b66fee7f8ee (:tipc: fix uninit-value in
tipc_nl_compat_link_reset_stats")
Sorry, David, do I need to resend this one?
^ permalink raw reply
* Re: [PATCH net-next] net: macb: use NAPI_POLL_WEIGHT
From: Nicolas.Ferre @ 2019-06-24 15:58 UTC (permalink / raw)
To: antoine.tenart, davem
Cc: netdev, linux-kernel, Ludovic.Desroches, alexandre.belloni
In-Reply-To: <20190621152855.30330-1-antoine.tenart@bootlin.com>
On 21/06/2019 at 17:28, Antoine Tenart wrote:
> Use NAPI_POLL_WEIGHT, the default NAPI poll() weight instead of
> redefining our own value (which turns out to be 64 as well).
>
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 163deba244ab..1cd1f2c36d6f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3490,7 +3490,7 @@ static int macb_init(struct platform_device *pdev)
>
> queue = &bp->queues[q];
> queue->bp = bp;
> - netif_napi_add(dev, &queue->napi, macb_poll, 64);
> + netif_napi_add(dev, &queue->napi, macb_poll, NAPI_POLL_WEIGHT);
> if (hw_q) {
> queue->ISR = GEM_ISR(hw_q - 1);
> queue->IER = GEM_IER(hw_q - 1);
>
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH net] net: macb: do not copy the mac address if NULL
From: David Miller @ 2019-06-24 15:57 UTC (permalink / raw)
To: antoine.tenart
Cc: nicolas.ferre, netdev, linux-kernel, ludovic.desroches,
alexandre.belloni
In-Reply-To: <20190621152635.29689-1-antoine.tenart@bootlin.com>
From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Fri, 21 Jun 2019 17:26:35 +0200
> This patch fixes the MAC address setup in the probe. The MAC address
> retrieved using of_get_mac_address was checked for not containing an
> error, but it may also be NULL which wasn't tested. Fix it by replacing
> IS_ERR with IS_ERR_OR_NULL.
>
> Fixes: 541ddc66d665 ("net: macb: support of_get_mac_address new ERR_PTR error")
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net] tipc: remove the unnecessary msg->req check from tipc_nl_compat_bearer_set
From: David Miller @ 2019-06-24 15:56 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, jon.maloy, ying.xue, tipc-discussion
In-Reply-To: <CADvbK_dcOYbFhiiseoAou_CAELLbta585S=g2jZ249ftO3h+2w@mail.gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 24 Jun 2019 23:54:02 +0800
> On Mon, Jun 24, 2019 at 10:35 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Xin Long <lucien.xin@gmail.com>
>> Date: Mon, 24 Jun 2019 16:02:42 +0800
>>
>> > tipc_nl_compat_bearer_set() is only called by tipc_nl_compat_link_set()
>> > which already does the check for msg->req check, so remove it from
>> > tipc_nl_compat_bearer_set(), and do the same in tipc_nl_compat_media_set().
>> >
>> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>
>> Is this really appropriate as a fix for 'net'? Seems more like net-next material
>> to me.
> kind of code fix, sure, you can apply it to net-next, no conflict.
> do you need me to repost?
No need to resend, applied to net-next, thanks!
^ permalink raw reply
* Re: [PATCH net] net: macb: do not copy the mac address if NULL
From: Nicolas.Ferre @ 2019-06-24 15:55 UTC (permalink / raw)
To: antoine.tenart, davem
Cc: netdev, linux-kernel, Ludovic.Desroches, alexandre.belloni
In-Reply-To: <20190621152635.29689-1-antoine.tenart@bootlin.com>
On 21/06/2019 at 17:26, Antoine Tenart wrote:
> This patch fixes the MAC address setup in the probe. The MAC address
> retrieved using of_get_mac_address was checked for not containing an
> error, but it may also be NULL which wasn't tested. Fix it by replacing
> IS_ERR with IS_ERR_OR_NULL.
>
> Fixes: 541ddc66d665 ("net: macb: support of_get_mac_address new ERR_PTR error")
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
It could be good to have this fix for 5.2-final...
Thanks!
> ---
> drivers/net/ethernet/cadence/macb_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 1241a2a73438..1cd1f2c36d6f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4304,7 +4304,7 @@ static int macb_probe(struct platform_device *pdev)
> if (PTR_ERR(mac) == -EPROBE_DEFER) {
> err = -EPROBE_DEFER;
> goto err_out_free_netdev;
> - } else if (!IS_ERR(mac)) {
> + } else if (!IS_ERR_OR_NULL(mac)) {
> ether_addr_copy(bp->dev->dev_addr, mac);
> } else {
> macb_get_hwaddr(bp);
>
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH net] tipc: remove the unnecessary msg->req check from tipc_nl_compat_bearer_set
From: Xin Long @ 2019-06-24 15:54 UTC (permalink / raw)
To: David Miller; +Cc: network dev, Jon Maloy, Ying Xue, tipc-discussion
In-Reply-To: <20190624.073517.1612706351643151777.davem@davemloft.net>
On Mon, Jun 24, 2019 at 10:35 PM David Miller <davem@davemloft.net> wrote:
>
> From: Xin Long <lucien.xin@gmail.com>
> Date: Mon, 24 Jun 2019 16:02:42 +0800
>
> > tipc_nl_compat_bearer_set() is only called by tipc_nl_compat_link_set()
> > which already does the check for msg->req check, so remove it from
> > tipc_nl_compat_bearer_set(), and do the same in tipc_nl_compat_media_set().
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> Is this really appropriate as a fix for 'net'? Seems more like net-next material
> to me.
kind of code fix, sure, you can apply it to net-next, no conflict.
do you need me to repost?
^ permalink raw reply
* Re: [PATCH v2 net-next 4/4] cxgb4: Add MPS refcounting for alloc/free mac filters
From: kbuild test robot @ 2019-06-24 15:51 UTC (permalink / raw)
To: Raju Rangoju; +Cc: kbuild-all, netdev, davem, nirranjan, dt, rajur
In-Reply-To: <20190624085037.2358-5-rajur@chelsio.com>
[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]
Hi Raju,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Raju-Rangoju/cxgb4-Reference-count-MPS-TCAM-entries-within-a-PF/20190624-230630
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/ethernet/chelsio/cxgb4/cxgb4_mps.c: In function 'cxgb4_mps_ref_dec_by_mac':
>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_mps.c:17:29: error: passing argument 1 of 'atomic_dec_and_test' from incompatible pointer type [-Werror=incompatible-pointer-types]
if (!atomic_dec_and_test(&mps_entry->refcnt)) {
^
In file included from include/linux/atomic.h:74:0,
from arch/ia64/include/asm/processor.h:79,
from arch/ia64/include/asm/thread_info.h:12,
from include/linux/thread_info.h:38,
from include/asm-generic/preempt.h:5,
from ./arch/ia64/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/hardirq.h:5,
from include/linux/interrupt.h:11,
from drivers/net/ethernet/chelsio/cxgb4/cxgb4.h:42,
from drivers/net/ethernet/chelsio/cxgb4/cxgb4_mps.c:4:
include/linux/atomic-fallback.h:1031:1: note: expected 'atomic_t * {aka struct <anonymous> *}' but argument is of type 'refcount_t * {aka struct refcount_struct *}'
atomic_dec_and_test(atomic_t *v)
^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/atomic_dec_and_test +17 drivers/net/ethernet/chelsio/cxgb4/cxgb4_mps.c
5
6 static int cxgb4_mps_ref_dec_by_mac(struct adapter *adap,
7 const u8 *addr, const u8 *mask)
8 {
9 u8 bitmask[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
10 struct mps_entries_ref *mps_entry, *tmp;
11 int ret = -EINVAL;
12
13 spin_lock_bh(&adap->mps_ref_lock);
14 list_for_each_entry_safe(mps_entry, tmp, &adap->mps_ref, list) {
15 if (ether_addr_equal(mps_entry->addr, addr) &&
16 ether_addr_equal(mps_entry->mask, mask ? mask : bitmask)) {
> 17 if (!atomic_dec_and_test(&mps_entry->refcnt)) {
18 spin_unlock_bh(&adap->mps_ref_lock);
19 return -EBUSY;
20 }
21 list_del(&mps_entry->list);
22 kfree(mps_entry);
23 ret = 0;
24 break;
25 }
26 }
27 spin_unlock_bh(&adap->mps_ref_lock);
28 return ret;
29 }
30
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54085 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] module: Fix up module_notifier return values.
From: Joel Fernandes @ 2019-06-24 15:52 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Frank Ch. Eigler, Jessica Yu, linux-kernel,
Josh Poimboeuf, jikos, mbenes, Petr Mladek, Alexei Starovoitov,
Daniel Borkmann, Andrew Morton, Robert Richter, rostedt,
Ingo Molnar, Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
Ard Biesheuvel, Thomas Gleixner, oprofile-list, netdev, bpf
In-Reply-To: <320564860.243.1561384864186.JavaMail.zimbra@efficios.com>
On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org wrote:
>
> > While auditing all module notifiers I noticed a whole bunch of fail
> > wrt the return value. Notifiers have a 'special' return semantics.
> >
> > Cc: Robert Richter <rric@kernel.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Martin KaFai Lau <kafai@fb.com>
> > Cc: Song Liu <songliubraving@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: oprofile-list@lists.sf.net
> > Cc: linux-kernel@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Cc: bpf@vger.kernel.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Thanks Peter for looking into this, especially considering your
> endless love for kernel modules! ;)
>
> It's not directly related to your changes, but I notice that
> kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
> appears to leak memory. Am I missing something ?
Could you elaborate? Do you mean there is no MODULE_STATE_GOING notifier
check? If that's what you mean then I agree, there should be some place
where the format structures are freed when the module is unloaded no?
>
> With respect to your changes:
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Looks good to me too.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Could we CC stable so that the fix is propagated to older kernels?
thanks,
- Joel
> I have a similar erroneous module notifier return value pattern
> in lttng-modules as well. I'll go fix it right away. CCing
> Frank Eigler from SystemTAP which AFAIK use a copy of
> lttng-tracepoint.c in their project, which should be fixed
> as well. I'm pasting the lttng-modules fix below.
>
> Thanks!
>
> Mathieu
>
> --
>
> commit 5eac9d146a7d947f0f314c4f7103c92cbccaeaf3
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date: Mon Jun 24 09:43:45 2019 -0400
>
> Fix: lttng-tracepoint module notifier should return NOTIFY_OK
>
> Module notifiers should return NOTIFY_OK on success rather than the
> value 0. The return value 0 does not seem to have any ill side-effects
> in the notifier chain caller, but it is preferable to respect the API
> requirements in case this changes in the future.
>
> Notifiers can encapsulate a negative errno value with
> notifier_from_errno(), but this is not needed by the LTTng tracepoint
> notifier.
>
> The approach taken in this notifier is to just print a console warning
> on error, because tracing failure should not prevent loading a module.
> So we definitely do not want to stop notifier iteration. Returning
> an error without stopping iteration is not really that useful, because
> only the return value of the last callback is returned to notifier chain
> caller.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>
> diff --git a/lttng-tracepoint.c b/lttng-tracepoint.c
> index bbb2c7a4..8298b397 100644
> --- a/lttng-tracepoint.c
> +++ b/lttng-tracepoint.c
> @@ -256,7 +256,7 @@ int lttng_tracepoint_coming(struct tp_module *tp_mod)
> }
> }
> mutex_unlock(<tng_tracepoint_mutex);
> - return 0;
> + return NOTIFY_OK;
> }
>
> static
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 2/2] net: macb: Kconfig: Rename Atmel to Cadence
From: Nicolas.Ferre @ 2019-06-24 15:42 UTC (permalink / raw)
To: palmer; +Cc: davem, netdev, linux-kernel
In-Reply-To: <mhng-87f5f418-acd4-4a29-b82e-6dc574a4828a@palmer-si-x1e>
On 24/06/2019 at 11:57, Palmer Dabbelt wrote:
> External E-Mail
>
>
> On Mon, 24 Jun 2019 02:49:16 PDT (-0700), Nicolas.Ferre@microchip.com wrote:
>> On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
>>> External E-Mail
>>>
>>>
>>> When touching the Kconfig for this driver I noticed that both the
>>> Kconfig help text and a comment referred to this being an Atmel driver.
>>> As far as I know, this is a Cadence driver. The fix is just
>>
>> Indeed: was written and then maintained by Atmel (now Microchip) for
>> years... So I would say that more than a "Cadence driver" it's a driver
>> that applies to a Cadence peripheral.
>>
>> I won't hold the patch just for this as the patch makes perfect sense,
>> but would love that it's been highlighted...
>
> OK, I don't mind changing it. Does this look OK? I have to submit a v2 anyway
> for the first patch.
Yep, nice!
Thanks,
Nicolas
>
> Author: Palmer Dabbelt <palmer@sifive.com>
> Date: Sun Jun 23 23:04:14 2019 -0700
>
> net: macb: Kconfig: Rename Atmel to Cadence
>
> The help text makes it look like NET_VENDOR_CADENCE enables support for
> Atmel devices, when in reality it's a driver written by Atmel that
> supports Cadence devices. This may confuse users that have this device
> on a non-Atmel SoC.
>
> The fix is just s/Atmel/Cadence/, but I did go and re-wrap the Kconfig
> help text as that change caused it to go over 80 characters.
>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>
> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index 74ee2bfd2369..29b6132b418e 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> #
> -# Atmel device configuration
> +# Cadence device configuration
> #
>
> config NET_VENDOR_CADENCE
> @@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
> If unsure, say Y.
>
> Note that the answer to this question doesn't directly affect the
> - kernel: saying N will just cause the configurator to skip all
> - the remaining Atmel network card questions. If you say Y, you will be
> + kernel: saying N will just cause the configurator to skip all the
> + remaining Cadence network card questions. If you say Y, you will be
> asked for your specific card in the following questions.
>
> if NET_VENDOR_CADENCE
>
>>
>>> s/Atmel/Cadence/, but I did go and re-wrap the Kconfig help text as that
>>> change caused it to go over 80 characters.
>>>
>>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>>> ---
>>> drivers/net/ethernet/cadence/Kconfig | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
>>> index 74ee2bfd2369..29b6132b418e 100644
>>> --- a/drivers/net/ethernet/cadence/Kconfig
>>> +++ b/drivers/net/ethernet/cadence/Kconfig
>>> @@ -1,6 +1,6 @@
>>> # SPDX-License-Identifier: GPL-2.0-only
>>> #
>>> -# Atmel device configuration
>>> +# Cadence device configuration
>>> #
>>>
>>> config NET_VENDOR_CADENCE
>>> @@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
>>> If unsure, say Y.
>>>
>>> Note that the answer to this question doesn't directly affect the
>>> - kernel: saying N will just cause the configurator to skip all
>>> - the remaining Atmel network card questions. If you say Y, you will be
>>> + kernel: saying N will just cause the configurator to skip all the
>>> + remaining Cadence network card questions. If you say Y, you will be
>>> asked for your specific card in the following questions.
>>>
>>> if NET_VENDOR_CADENCE
>>>
>>
>>
>> --
>> Nicolas Ferre
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-24 15:39 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624145251.4849-2-opensource@vdorst.com>
Hi,
On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
> Convert mt7530 to PHYLINK API
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
> drivers/net/dsa/mt7530.h | 9 ++
> 2 files changed, 187 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3181e95586d6..9c5e4dd00826 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -13,7 +13,7 @@
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> #include <linux/of_platform.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
> return ARRAY_SIZE(mt7530_mib);
> }
>
> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
> - struct phy_device *phydev)
> -{
> - struct mt7530_priv *priv = ds->priv;
> -
> - if (phy_is_pseudo_fixed_link(phydev)) {
> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
> - phydev->interface);
> -
> - /* Setup TX circuit incluing relevant PAD and driving */
> - mt7530_pad_clk_setup(ds, phydev->interface);
> -
> - if (priv->id == ID_MT7530) {
> - /* Setup RX circuit, relevant PAD and driving on the
> - * host which must be placed after the setup on the
> - * device side is all finished.
> - */
> - mt7623_pad_clk_setup(ds);
> - }
> - } else {
> - u16 lcl_adv = 0, rmt_adv = 0;
> - u8 flowctrl;
> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
> -
> - switch (phydev->speed) {
> - case SPEED_1000:
> - mcr |= PMCR_FORCE_SPEED_1000;
> - break;
> - case SPEED_100:
> - mcr |= PMCR_FORCE_SPEED_100;
> - break;
> - }
> -
> - if (phydev->link)
> - mcr |= PMCR_FORCE_LNK;
> -
> - if (phydev->duplex) {
> - mcr |= PMCR_FORCE_FDX;
> -
> - if (phydev->pause)
> - rmt_adv = LPA_PAUSE_CAP;
> - if (phydev->asym_pause)
> - rmt_adv |= LPA_PAUSE_ASYM;
> -
> - lcl_adv = linkmode_adv_to_lcl_adv_t(
> - phydev->advertising);
> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
> -
> - if (flowctrl & FLOW_CTRL_TX)
> - mcr |= PMCR_TX_FC_EN;
> - if (flowctrl & FLOW_CTRL_RX)
> - mcr |= PMCR_RX_FC_EN;
> - }
> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> - }
> -}
> -
> static int
> mt7530_cpu_port_enable(struct mt7530_priv *priv,
> int port)
> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
> return 0;
> }
>
> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
> +
> + switch (port) {
> + case 0: /* Internal phy */
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + if (state->interface != PHY_INTERFACE_MODE_GMII)
> + goto unsupported;
> + break;
> + /* case 5: Port 5 is not supported! */
> + case 6: /* 1st cpu port */
> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> + state->interface != PHY_INTERFACE_MODE_TRGMII)
> + goto unsupported;
> +
> + /* Setup TX circuit incluing relevant PAD and driving */
> + mt7530_pad_clk_setup(ds, state->interface);
> +
> + if (priv->id == ID_MT7530) {
> + /* Setup RX circuit, relevant PAD and driving on the
> + * host which must be placed after the setup on the
> + * device side is all finished.
> + */
> + mt7623_pad_clk_setup(ds);
> + }
> + break;
> + default:
> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> + return;
> + }
> +
> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
> + mcr |= PMCR_FORCE_MODE;
> +
> + if (state->speed == SPEED_1000)
> + mcr |= PMCR_FORCE_SPEED_1000;
> + if (state->speed == SPEED_100)
> + mcr |= PMCR_FORCE_SPEED_100;
> + if (state->duplex == DUPLEX_FULL)
> + mcr |= PMCR_FORCE_FDX;
> + if (state->link || mode == MLO_AN_FIXED)
> + mcr |= PMCR_FORCE_LNK;
This should be removed - state->link is not for use in mac_config.
Even in fixed mode, the link can be brought up/down by means of a
gpio, and this should be dealt with via the mac_link_* functions.
> + if (state->pause || phylink_test(state->advertising, Pause))
> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> + if (state->pause & MLO_PAUSE_TX)
> + mcr |= PMCR_TX_FC_EN;
> + if (state->pause & MLO_PAUSE_RX)
> + mcr |= PMCR_RX_FC_EN;
This is clearly wrong - if any bit in state->pause is set, then we
end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
Pause set in the advertising mask, then both are set. This doesn't
seem right - are these bits setting the advertisement, or are they
telling the MAC to use flow control?
> + }
> +
> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> +
> + return;
> +
> +unsupported:
> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
> + __func__, port, state->interface, phy_modes(state->interface));
> +}
> +
> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface)
> +{
> + /* Do nothing */
> +}
> +
> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev)
> +{
> + /* Do nothing */
> +}
These two are where you should be forcing the link up or down if
required (basically, inband modes should let the link come up/down
irrespective of these functions, otherwise it should be forced.)
> +
> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + switch (port) {
> + case 0: /* Internal phy */
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + if (state->interface != PHY_INTERFACE_MODE_NA &&
> + state->interface != PHY_INTERFACE_MODE_GMII)
> + goto unsupported;
> + break;
> + /* case 5: Port 5 not supported! */
> + case 6: /* 1st cpu port */
> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> + state->interface != PHY_INTERFACE_MODE_TRGMII)
PHY_INTERFACE_MODE_NA ?
> + goto unsupported;
> + break;
> + default:
> + linkmode_zero(supported);
> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> + return;
> + }
> +
> + phylink_set(mask, Autoneg);
> + phylink_set(mask, Pause);
> + phylink_set(mask, Asym_Pause);
> + phylink_set(mask, MII);
> +
> + phylink_set(mask, 10baseT_Half);
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Half);
> + phylink_set(mask, 100baseT_Full);
> + phylink_set(mask, 1000baseT_Full);
> + phylink_set(mask, 1000baseT_Half);
You seem to be missing phylink_set_port_modes() here.
> +
> + linkmode_and(supported, supported, mask);
> + linkmode_and(state->advertising, state->advertising, mask);
> + return;
> +
> +unsupported:
> + linkmode_zero(supported);
> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
> + __func__, state->interface, phy_modes(state->interface));
Not a good idea to print this at error level; sometimes we just probe
for support.
Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
interface mode that the MAC can't support - we detect that by the
validation failing, and printing a more meaningful message in phylink
itself.
> +}
> +
> +static int
> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
> + struct phylink_link_state *state)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + u32 pmsr;
> +
> + if (port < 0 || port >= MT7530_NUM_PORTS)
> + return -EINVAL;
> +
> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
> +
> + state->link = (pmsr & PMSR_LINK);
> + state->an_complete = state->link;
> + state->duplex = (pmsr & PMSR_DPX) >> 1;
> +
> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
> + case 0:
> + state->speed = SPEED_10;
> + break;
> + case PMSR_SPEED_100:
> + state->speed = SPEED_100;
> + break;
> + case PMSR_SPEED_1000:
> + state->speed = SPEED_1000;
> + break;
> + default:
> + state->speed = SPEED_UNKNOWN;
> + break;
> + }
> +
> + state->pause = 0;
> + if (pmsr & PMSR_RX_FC)
> + state->pause |= MLO_PAUSE_RX;
> + if (pmsr & PMSR_TX_FC)
> + state->pause |= MLO_PAUSE_TX;
> +
> + return 1;
> +}
> +
> static const struct dsa_switch_ops mt7530_switch_ops = {
> .get_tag_protocol = mtk_get_tag_protocol,
> .setup = mt7530_setup,
> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
> .phy_write = mt7530_phy_write,
> .get_ethtool_stats = mt7530_get_ethtool_stats,
> .get_sset_count = mt7530_get_sset_count,
> - .adjust_link = mt7530_adjust_link,
> .port_enable = mt7530_port_enable,
> .port_disable = mt7530_port_disable,
> .port_stp_state_set = mt7530_stp_state_set,
> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
> .port_vlan_prepare = mt7530_port_vlan_prepare,
> .port_vlan_add = mt7530_port_vlan_add,
> .port_vlan_del = mt7530_port_vlan_del,
> + .phylink_validate = mt7530_phylink_validate,
> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
> + .phylink_mac_config = mt7530_phylink_mac_config,
> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
> };
>
> static const struct of_device_id mt7530_of_match[] = {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index bfac90f48102..41d9a132ac70 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
> #define PMCR_FORCE_SPEED_100 BIT(2)
> #define PMCR_FORCE_FDX BIT(1)
> #define PMCR_FORCE_LNK BIT(0)
> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
> PMCR_TX_EN | PMCR_RX_EN | \
> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>
> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
> +#define PMSR_EEE1G BIT(7)
> +#define PMSR_EEE100M BIT(6)
> +#define PMSR_RX_FC BIT(5)
> +#define PMSR_TX_FC BIT(4)
> +#define PMSR_SPEED_1000 BIT(3)
> +#define PMSR_SPEED_100 BIT(2)
> +#define PMSR_DPX BIT(1)
> +#define PMSR_LINK BIT(0)
>
> /* Register for MIB */
> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
> --
> 2.20.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Björn Töpel @ 2019-06-24 15:38 UTC (permalink / raw)
To: Kevin Laatz
Cc: Netdev, Alexei Starovoitov, Daniel Borkmann,
Björn Töpel, Karlsson, Magnus, bpf, intel-wired-lan,
Bruce Richardson, ciara.loftus
In-Reply-To: <20190620083924.1996-1-kevin.laatz@intel.com>
On Thu, 20 Jun 2019 at 18:55, Kevin Laatz <kevin.laatz@intel.com> wrote:
>
> This patchset adds the ability to use unaligned chunks in the XDP umem.
>
> Currently, all chunk addresses passed to the umem are masked to be chunk
> size aligned (default is 2k, max is PAGE_SIZE). This limits where we can
> place chunks within the umem as well as limiting the packet sizes that are
> supported.
>
> The changes in this patchset removes these restrictions, allowing XDP to be
> more flexible in where it can place a chunk within a umem. By relaxing where
> the chunks can be placed, it allows us to use an arbitrary buffer size and
> place that wherever we have a free address in the umem. These changes add the
> ability to support jumboframes and make it easy to integrate with other
> existing frameworks that have their own memory management systems, such as
> DPDK.
>
Thanks for working on this, Kevin and Ciara!
I have some minor comments on the series, but in general I think it's
in good shape!
For some reason the series was submitted twice (at least on my side)?
Thanks,
Björn
> Structure of the patchset:
> Patch 1:
> - Remove unnecessary masking and headroom addition during zero-copy Rx
> buffer recycling in i40e. This change is required in order for the
> buffer recycling to work in the unaligned chunk mode.
>
> Patch 2:
> - Remove unnecessary masking and headroom addition during
> zero-copy Rx buffer recycling in ixgbe. This change is required in
> order for the buffer recycling to work in the unaligned chunk mode.
>
> Patch 3:
> - Adds an offset parameter to zero_copy_allocator. This change will
> enable us to calculate the original handle in zca_free. This will be
> required for unaligned chunk mode since we can't easily mask back to
> the original handle.
>
> Patch 4:
> - Adds the offset parameter to i40e_zca_free. This change is needed for
> calculating the handle since we can't easily mask back to the original
> handle like we can in the aligned case.
>
> Patch 5:
> - Adds the offset parameter to ixgbe_zca_free. This change is needed for
> calculating the handle since we can't easily mask back to the original
> handle like we can in the aligned case.
>
>
> Patch 6:
> - Add infrastructure for unaligned chunks. Since we are dealing
> with unaligned chunks that could potentially cross a physical page
> boundary, we add checks to keep track of that information. We can
> later use this information to correctly handle buffers that are
> placed at an address where they cross a page boundary.
>
> Patch 7:
> - Add flags for umem configuration to libbpf
>
> Patch 8:
> - Modify xdpsock application to add a command line option for
> unaligned chunks
>
> Patch 9:
> - Addition of command line argument to pass in a desired buffer size
> and buffer recycling for unaligned mode. Passing in a buffer size will
> allow the application to use unaligned chunks with the unaligned chunk
> mode. Since we are now using unaligned chunks, we need to recycle our
> buffers in a slightly different way.
>
> Patch 10:
> - Adds hugepage support to the xdpsock application
>
> Patch 11:
> - Documentation update to include the unaligned chunk scenario. We need
> to explicitly state that the incoming addresses are only masked in the
> aligned chunk mode and not the unaligned chunk mode.
>
> Kevin Laatz (11):
> i40e: simplify Rx buffer recycle
> ixgbe: simplify Rx buffer recycle
> xdp: add offset param to zero_copy_allocator
> i40e: add offset to zca_free
> ixgbe: add offset to zca_free
> xsk: add support to allow unaligned chunk placement
> libbpf: add flags to umem config
> samples/bpf: add unaligned chunks mode support to xdpsock
> samples/bpf: add buffer recycling for unaligned chunks to xdpsock
> samples/bpf: use hugepages in xdpsock app
> doc/af_xdp: include unaligned chunk case
>
> Documentation/networking/af_xdp.rst | 10 +-
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 21 ++--
> drivers/net/ethernet/intel/i40e/i40e_xsk.h | 3 +-
> .../ethernet/intel/ixgbe/ixgbe_txrx_common.h | 3 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 21 ++--
> include/net/xdp.h | 3 +-
> include/net/xdp_sock.h | 2 +
> include/uapi/linux/if_xdp.h | 4 +
> net/core/xdp.c | 11 ++-
> net/xdp/xdp_umem.c | 17 ++--
> net/xdp/xsk.c | 60 +++++++++--
> net/xdp/xsk_queue.h | 60 +++++++++--
> samples/bpf/xdpsock_user.c | 99 ++++++++++++++-----
> tools/include/uapi/linux/if_xdp.h | 4 +
> tools/lib/bpf/xsk.c | 7 ++
> tools/lib/bpf/xsk.h | 2 +
> 16 files changed, 241 insertions(+), 86 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 1/2] net/macb: bindings doc: add sifive fu540-c000 binding
From: Nicolas.Ferre @ 2019-06-24 15:38 UTC (permalink / raw)
To: robh+dt, yash.shah
Cc: davem, netdev, linux-kernel, linux-riscv, devicetree,
mark.rutland, palmer, aou, ynezz, paul.walmsley, sachin.ghadi
In-Reply-To: <CAL_Jsq+p5PnTDgxuh9_Aw1RvTk4aTYjKxyMq7DPczLzQVv8_ew@mail.gmail.com>
On 23/05/2019 at 22:50, Rob Herring wrote:
> On Thu, May 23, 2019 at 6:46 AM Yash Shah <yash.shah@sifive.com> wrote:
>>
>> Add the compatibility string documentation for SiFive FU540-C0000
>> interface.
>> On the FU540, this driver also needs to read and write registers in a
>> management IP block that monitors or drives boundary signals for the
>> GEMGXL IP block that are not directly mapped to GEMGXL registers.
>> Therefore, add additional range to "reg" property for SiFive GEMGXL
>> management IP registers.
>>
>> Signed-off-by: Yash Shah <yash.shah@sifive.com>
>> ---
>> Documentation/devicetree/bindings/net/macb.txt | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
>> index 9c5e944..91a2a66 100644
>> --- a/Documentation/devicetree/bindings/net/macb.txt
>> +++ b/Documentation/devicetree/bindings/net/macb.txt
>> @@ -4,6 +4,7 @@ Required properties:
>> - compatible: Should be "cdns,[<chip>-]{macb|gem}"
>> Use "cdns,at91rm9200-emac" Atmel at91rm9200 SoC.
>> Use "cdns,at91sam9260-macb" for Atmel at91sam9 SoCs.
>> + Use "cdns,fu540-macb" for SiFive FU540-C000 SoC.
>
> This pattern that Atmel started isn't really correct. The vendor
> prefix here should be sifive. 'cdns' would be appropriate for a
> fallback.
Ok, we missed this for the sam9x60 SoC that we added recently then.
Anyway a little too late, coming back to this machine, and talking to
Yash, isn't "sifive,fu540-c000-macb" more specific and a better match
for being future proof? I would advice for the most specific possible
with other compatible strings on the same line in the DT, like:
"sifive,fu540-c000-macb", "sifive,fu540-macb"
Moreover, is it really a "macb" or a "gem" type of interface from
Cadence? Not a big deal, but just to discuss the topic to the bone...
Note that I'm fine if you consider that what you have in net-next new is
correct.
Regards,
Nicolas
>> Use "cdns,sam9x60-macb" for Microchip sam9x60 SoC.
>> Use "cdns,np4-macb" for NP4 SoC devices.
>> Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb".
>> @@ -17,6 +18,8 @@ Required properties:
>> Use "cdns,zynqmp-gem" for Zynq Ultrascale+ MPSoC.
>> Or the generic form: "cdns,emac".
>> - reg: Address and length of the register set for the device
>> + For "cdns,fu540-macb", second range is required to specify the
>> + address and length of the registers for GEMGXL Management block.
>> - interrupts: Should contain macb interrupt
>> - phy-mode: See ethernet.txt file in the same directory.
>> - clock-names: Tuple listing input clock names.
>> --
>> 1.9.1
>>
>
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH V34 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Daniel Borkmann @ 2019-06-24 15:15 UTC (permalink / raw)
To: Matthew Garrett, jmorris
Cc: linux-security-module, linux-kernel, linux-api, David Howells,
Alexei Starovoitov, Matthew Garrett, netdev, Chun-Yi Lee, jannh,
bpf
In-Reply-To: <20190622000358.19895-24-matthewgarrett@google.com>
On 06/22/2019 02:03 AM, Matthew Garrett wrote:
> From: David Howells <dhowells@redhat.com>
>
> There are some bpf functions can be used to read kernel memory:
Nit: that
> bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow
Please explain how bpf_probe_write_user reads kernel memory ... ?!
> private keys in kernel memory (e.g. the hibernation image signing key) to
> be read by an eBPF program and kernel memory to be altered without
... and while we're at it, also how they allow "kernel memory to be
altered without restriction". I've been pointing this false statement
out long ago.
> restriction. Disable them if the kernel has been locked down in
> confidentiality mode.
>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> cc: netdev@vger.kernel.org
> cc: Chun-Yi Lee <jlee@suse.com>
> cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
Nacked-by: Daniel Borkmann <daniel@iogearbox.net>
[...]
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d64c00afceb5..638f9b00a8df 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -137,6 +137,10 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> {
> int ret;
>
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
This whole thing is still buggy as has been pointed out before by
Jann. For helpers like above and few others below, error conditions
must clear the buffer ...
> ret = probe_kernel_read(dst, unsafe_ptr, size);
> if (unlikely(ret < 0))
> memset(dst, 0, size);
> @@ -156,6 +160,12 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
> BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
> u32, size)
> {
> + int ret;
> +
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
> +
> /*
> * Ensure we're in user context which is safe for the helper to
> * run. This helper has no business in a kthread.
> @@ -205,7 +215,11 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
> int fmt_cnt = 0;
> u64 unsafe_addr;
> char buf[64];
> - int i;
> + int i, ret;
> +
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
>
> /*
> * bpf_check()->check_func_arg()->check_stack_boundary()
> @@ -534,6 +548,10 @@ BPF_CALL_3(bpf_probe_read_str, void *, dst, u32, size,
> {
> int ret;
>
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
> +
> /*
> * The strncpy_from_unsafe() call will likely not fill the entire
> * buffer, but that's okay in this circumstance as we're probing
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 5a08c17f224d..2eea2cc13117 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -33,6 +33,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> [LOCKDOWN_KCORE] = "/proc/kcore access",
> [LOCKDOWN_KPROBES] = "use of kprobes",
> + [LOCKDOWN_BPF_READ] = "use of bpf to read kernel RAM",
> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> };
>
>
^ permalink raw reply
* Re: [PATCH][next] libbpf: fix spelling mistake "conflictling" -> "conflicting"
From: Daniel Borkmann @ 2019-06-24 14:59 UTC (permalink / raw)
To: Colin King, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, netdev, bpf
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190619162742.985-1-colin.king@canonical.com>
On 06/19/2019 06:27 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There are several spelling mistakes in pr_warning messages. Fix these.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH 10/11] samples/bpf: use hugepages in xdpsock app
From: Björn Töpel @ 2019-06-24 15:36 UTC (permalink / raw)
To: Kevin Laatz
Cc: Netdev, Alexei Starovoitov, Daniel Borkmann,
Björn Töpel, Karlsson, Magnus, bpf, intel-wired-lan,
Bruce Richardson, ciara.loftus
In-Reply-To: <20190620090958.2135-11-kevin.laatz@intel.com>
On Thu, 20 Jun 2019 at 19:25, Kevin Laatz <kevin.laatz@intel.com> wrote:
>
> This patch modifies xdpsock to use mmap instead of posix_memalign. With
> this change, we can use hugepages when running the application in unaligned
> chunks mode. Using hugepages makes it more likely that we have physically
> contiguous memory, which supports the unaligned chunk mode better.
>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> samples/bpf/xdpsock_user.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
> index 7b4ce047deb2..8ed63ad68428 100644
> --- a/samples/bpf/xdpsock_user.c
> +++ b/samples/bpf/xdpsock_user.c
> @@ -74,6 +74,7 @@ static int opt_interval = 1;
> static u64 opt_buffer_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> static u32 opt_umem_flags;
> static int opt_unaligned_chunks;
> +static int opt_mmap_flags;
> static u32 opt_xdp_bind_flags;
> static __u32 prog_id;
>
> @@ -438,6 +439,7 @@ static void parse_command_line(int argc, char **argv)
> case 'u':
> opt_umem_flags |= XDP_UMEM_UNALIGNED_CHUNKS;
> opt_unaligned_chunks = 1;
> + opt_mmap_flags = MAP_HUGETLB;
> break;
> case 'b':
> opt_buffer_size = atoi(optarg);
> @@ -707,11 +709,13 @@ int main(int argc, char **argv)
> exit(EXIT_FAILURE);
> }
>
> - ret = posix_memalign(&bufs, getpagesize(), /* PAGE_SIZE aligned */
> - NUM_FRAMES * opt_buffer_size);
> - if (ret)
> - exit_with_error(ret);
> -
> + /* Reserve memory for the umem. Use hugepages if unaligned chunk mode */
> + bufs = mmap(NULL, NUM_FRAMES * opt_buffer_size, PROT_READ|PROT_WRITE,
> + MAP_PRIVATE|MAP_ANONYMOUS|opt_mmap_flags, -1, 0);
> + if (bufs == MAP_FAILED) {
> + printf("ERROR: mmap failed\n");
> + exit(EXIT_FAILURE);
> + }
> /* Create sockets... */
> umem = xsk_configure_umem(bufs,
> NUM_FRAMES * opt_buffer_size);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 09/11] samples/bpf: add buffer recycling for unaligned chunks to xdpsock
From: Björn Töpel @ 2019-06-24 15:35 UTC (permalink / raw)
To: Kevin Laatz
Cc: Netdev, Alexei Starovoitov, Daniel Borkmann,
Björn Töpel, Karlsson, Magnus, bpf, intel-wired-lan,
Bruce Richardson, ciara.loftus
In-Reply-To: <20190620090958.2135-10-kevin.laatz@intel.com>
On Thu, 20 Jun 2019 at 19:25, Kevin Laatz <kevin.laatz@intel.com> wrote:
>
> This patch adds buffer recycling support for unaligned buffers. Since we
> don't mask the addr to 2k at umem_teg in unaligned mode, we need to make
> sure we give back the correct, original addr to the fill queue. To do this,
> we need to mask the addr with the buffer size.
>
> To pass in a buffer size, use the --buf-size=n argument.
> NOTE: For xdpsock to work in aligned chunk mode, you still need to pass
> 'power of 2' buffer size.
>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
> samples/bpf/xdpsock_user.c | 71 +++++++++++++++++++++++++++-----------
> 1 file changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
> index e26f43382d01..7b4ce047deb2 100644
> --- a/samples/bpf/xdpsock_user.c
> +++ b/samples/bpf/xdpsock_user.c
> @@ -60,6 +60,10 @@ enum benchmark_type {
> BENCH_L2FWD = 2,
> };
>
> +#define LENGTH (256UL*1024*1024)
> +#define ADDR (void *)(0x0UL)
> +#define SHMAT_FLAGS (0)
Not used.
> +
> static enum benchmark_type opt_bench = BENCH_RXDROP;
> static u32 opt_xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
> static const char *opt_if = "";
> @@ -67,6 +71,7 @@ static int opt_ifindex;
> static int opt_queue;
> static int opt_poll;
> static int opt_interval = 1;
> +static u64 opt_buffer_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> static u32 opt_umem_flags;
> static int opt_unaligned_chunks;
> static u32 opt_xdp_bind_flags;
> @@ -287,7 +292,7 @@ static struct xsk_umem_info *xsk_configure_umem(void *buffer, u64 size)
>
> umem_cfg.fill_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
> umem_cfg.comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> - umem_cfg.frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> + umem_cfg.frame_size = opt_buffer_size;
> umem_cfg.frame_headroom = XSK_UMEM__DEFAULT_FRAME_HEADROOM;
> umem_cfg.flags = opt_umem_flags;
>
> @@ -334,8 +339,8 @@ static struct xsk_socket_info *xsk_configure_socket(struct xsk_umem_info *umem)
> exit_with_error(-ret);
> for (i = 0;
> i < XSK_RING_PROD__DEFAULT_NUM_DESCS *
> - XSK_UMEM__DEFAULT_FRAME_SIZE;
> - i += XSK_UMEM__DEFAULT_FRAME_SIZE)
> + opt_buffer_size;
> + i += opt_buffer_size)
> *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx++) = i;
> xsk_ring_prod__submit(&xsk->umem->fq,
> XSK_RING_PROD__DEFAULT_NUM_DESCS);
> @@ -356,6 +361,7 @@ static struct option long_options[] = {
> {"zero-copy", no_argument, 0, 'z'},
> {"copy", no_argument, 0, 'c'},
> {"unaligned", no_argument, 0, 'u'},
> + {"buf-size", required_argument, 0, 'b'},
> {0, 0, 0, 0}
> };
>
> @@ -376,6 +382,7 @@ static void usage(const char *prog)
> " -z, --zero-copy Force zero-copy mode.\n"
> " -c, --copy Force copy mode.\n"
> " -u, --unaligned Enable unaligned chunk placement\n"
> + " -b, --buf-size=n Specify the buffer size to use\n"
> "\n";
> fprintf(stderr, str, prog);
> exit(EXIT_FAILURE);
> @@ -388,7 +395,7 @@ static void parse_command_line(int argc, char **argv)
> opterr = 0;
>
> for (;;) {
> - c = getopt_long(argc, argv, "Frtli:q:psSNn:czu", long_options,
> + c = getopt_long(argc, argv, "Frtli:q:psSNn:czub", long_options,
> &option_index);
> if (c == -1)
> break;
> @@ -432,6 +439,9 @@ static void parse_command_line(int argc, char **argv)
> opt_umem_flags |= XDP_UMEM_UNALIGNED_CHUNKS;
> opt_unaligned_chunks = 1;
> break;
> + case 'b':
> + opt_buffer_size = atoi(optarg);
> + break;
> case 'F':
> opt_xdp_flags &= ~XDP_FLAGS_UPDATE_IF_NOEXIST;
> break;
> @@ -483,13 +493,22 @@ static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
> while (ret != rcvd) {
> if (ret < 0)
> exit_with_error(-ret);
> - ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd,
> - &idx_fq);
> + ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
> + }
> +
> + if (opt_umem_flags & XDP_UMEM_UNALIGNED_CHUNKS) {
> + for (i = 0; i < rcvd; i++) {
> + u64 comp_addr = *xsk_ring_cons__comp_addr(&xsk->umem->cq,
> + idx_cq++);
> + u64 masked_comp = (comp_addr & ~((u64)opt_buffer_size-1));
> + *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) =
> + masked_comp;
> + }
> + } else {
> + for (i = 0; i < rcvd; i++)
> + *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) =
> + *xsk_ring_cons__comp_addr(&xsk->umem->cq, idx_cq++);
> }
> - for (i = 0; i < rcvd; i++)
> - *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) =
> - *xsk_ring_cons__comp_addr(&xsk->umem->cq,
> - idx_cq++);
>
> xsk_ring_prod__submit(&xsk->umem->fq, rcvd);
> xsk_ring_cons__release(&xsk->umem->cq, rcvd);
> @@ -533,13 +552,25 @@ static void rx_drop(struct xsk_socket_info *xsk)
> ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
> }
>
> - for (i = 0; i < rcvd; i++) {
> - u64 addr = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx)->addr;
> - u32 len = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx++)->len;
> - char *pkt = xsk_umem__get_data(xsk->umem->buffer, addr);
> + if (opt_umem_flags & XDP_UMEM_UNALIGNED_CHUNKS) {
> + for (i = 0; i < rcvd; i++) {
> + u64 addr = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx)->addr;
> + u32 len = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx++)->len;
> + char *pkt = xsk_umem__get_data(xsk->umem->buffer, addr);
> + u64 masked_addr = (addr & ~((u64)opt_buffer_size-1));
> +
> + hex_dump(pkt, len, addr);
> + *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) = masked_addr;
> + }
> + } else {
> + for (i = 0; i < rcvd; i++) {
> + u64 addr = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx)->addr;
> + u32 len = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx++)->len;
> + char *pkt = xsk_umem__get_data(xsk->umem->buffer, addr);
>
> - hex_dump(pkt, len, addr);
> - *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) = addr;
> + hex_dump(pkt, len, addr);
> + *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) = addr;
> + }
> }
>
> xsk_ring_prod__submit(&xsk->umem->fq, rcvd);
> @@ -677,20 +708,20 @@ int main(int argc, char **argv)
> }
>
> ret = posix_memalign(&bufs, getpagesize(), /* PAGE_SIZE aligned */
> - NUM_FRAMES * XSK_UMEM__DEFAULT_FRAME_SIZE);
> + NUM_FRAMES * opt_buffer_size);
> if (ret)
> exit_with_error(ret);
>
> /* Create sockets... */
> umem = xsk_configure_umem(bufs,
> - NUM_FRAMES * XSK_UMEM__DEFAULT_FRAME_SIZE);
> + NUM_FRAMES * opt_buffer_size);
> xsks[num_socks++] = xsk_configure_socket(umem);
>
> if (opt_bench == BENCH_TXONLY) {
> int i;
>
> - for (i = 0; i < NUM_FRAMES * XSK_UMEM__DEFAULT_FRAME_SIZE;
> - i += XSK_UMEM__DEFAULT_FRAME_SIZE)
> + for (i = 0; i < NUM_FRAMES * opt_buffer_size;
> + i += opt_buffer_size)
> (void)gen_eth_frame(umem, i);
> }
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 11/11] doc/af_xdp: include unaligned chunk case
From: Björn Töpel @ 2019-06-24 15:34 UTC (permalink / raw)
To: Kevin Laatz
Cc: Netdev, Alexei Starovoitov, Daniel Borkmann,
Björn Töpel, Karlsson, Magnus, bpf, intel-wired-lan,
Bruce Richardson, ciara.loftus
In-Reply-To: <20190620090958.2135-12-kevin.laatz@intel.com>
On Thu, 20 Jun 2019 at 19:25, Kevin Laatz <kevin.laatz@intel.com> wrote:
>
> The addition of unaligned chunks mode, the documentation needs to be
> updated to indicate that the incoming addr to the fill ring will only be
> masked if the user application is run in the aligned chunk mode. This patch
> also adds a line to explicitly indicate that the incoming addr will not be
> masked if running the user application in the unaligned chunk mode.
>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> Documentation/networking/af_xdp.rst | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/networking/af_xdp.rst b/Documentation/networking/af_xdp.rst
> index e14d7d40fc75..16fbc68cac50 100644
> --- a/Documentation/networking/af_xdp.rst
> +++ b/Documentation/networking/af_xdp.rst
> @@ -153,10 +153,12 @@ an example, if the UMEM is 64k and each chunk is 4k, then the UMEM has
>
> Frames passed to the kernel are used for the ingress path (RX rings).
>
> -The user application produces UMEM addrs to this ring. Note that the
> -kernel will mask the incoming addr. E.g. for a chunk size of 2k, the
> -log2(2048) LSB of the addr will be masked off, meaning that 2048, 2050
> -and 3000 refers to the same chunk.
> +The user application produces UMEM addrs to this ring. Note that, if
> +running the application with aligned chunk mode, the kernel will mask
> +the incoming addr. E.g. for a chunk size of 2k, the log2(2048) LSB of
> +the addr will be masked off, meaning that 2048, 2050 and 3000 refers
> +to the same chunk. If the user application is run in the unaligned
> +chunks mode, then the incoming addr will be left untouched.
>
>
> UMEM Completion Ring
> --
> 2.17.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