* [PATCH net-next 00/10] ibmvnic: Updates and bug fixes
From: Nathan Fontenot @ 2017-04-19 17:44 UTC (permalink / raw)
To: netdev; +Cc: brking, jallen, muvic, tlfalcon
This set of patches is a series of updates to remove some unneeded
and unused code in the driver as well as bug fixes for the
ibmvnic driver.
---
Brian King (4):
ibmvnic: Unmap longer term buffer before free
ibmvnic: Fixup atomic API usage
ibmvnic: Do not disable IRQ after scheduling tasklet
ibmvnic: Disable irq prior to close
Murilo Fossa Vicentini (1):
ibmvnic: Fix ibmvnic_change_mac_addr struct format
Nathan Fontenot (4):
ibmvnic: Remove inflight list
ibmvnic: Correct crq and resource releasing
ibmvnic: Allocate zero-filled memory for sub crqs
ibmvnic: Remove unused bouce buffer
Thomas Falcon (1):
ibmvnic: Report errors when failing to release sub-crqs
drivers/net/ethernet/ibm/ibmvnic.c | 223 ++++++++++--------------------------
drivers/net/ethernet/ibm/ibmvnic.h | 15 --
2 files changed, 61 insertions(+), 177 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Eric Dumazet @ 2017-04-19 13:47 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <b318fd94-d4f5-d1b8-c113-a5cf374a6fee@mojatatu.com>
On Wed, 2017-04-19 at 07:24 -0400, Jamal Hadi Salim wrote:
> On 17-04-18 11:17 PM, Eric Dumazet wrote:
> > On Tue, 2017-04-18 at 22:32 -0400, Jamal Hadi Salim wrote:
> >> On 17-04-18 09:49 PM, Eric Dumazet wrote:
> >>> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:
>
> >>
> >> Make sense?
> >
> > What if we have 1024 actions, and user provides a 4KB buffer ?
> >
>
> No problem - we will fit as many per batch to consume 4KB
> and will send them as long as user calls recvmsg.
>
> > Normally multiple recvmsg() calls would be needed, but I do not see how
> > the nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) can always succeed.
>
> Oh, I see the cross-talk Eric;->
> We dont pack the actions in TCAA_ACT_COUNT - we put them in
> TCAA_ACT_TAB.
>
> Here's some strace capture that best describes what happened before
> and after which i hope will make sense. Granted tc uses 32KB from
> user space and not 4KB you mention.
>
> We have 400 actions in the kernel at this point:
>
> tc with no changes (doesnt for this large dump):
> ---
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"...,
> 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744
>
> ===> total actions dumped 29
>
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"
> \20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"...,
> 32768}], msg_controllen=0, msg_flags=0}, 0) = 4128
>
> ==> total actions dumped 32
>
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"
> \20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"...,
> 32768}], msg_controllen=0, msg_flags=0}, 0) = 4128
>
> ==> total actions dumped 32
> ....
> .....
> .........
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}],
> msg_controllen=0, msg_flags=0}, 0) = 20
> -----
>
> Goes on a few times until we get all 400 entries - last recvmsg (with
> 20B) has no actions and indicates dump is complete.
> Issue: The kernel is refusing to add more than 32 entries in the skb
> even though we get allocated 32KB for the skb.
>
> So now lets see what happens with this change:
> ------
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"...,
> 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744
>
> ==> total actions dumped 29
>
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"\240~\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204~\1\0\200\0\0\0\t\0\1\0"...,
> 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 32416
>
> ==> total actions dumped 253
>
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"
> ;\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\4;\1\0\200\0\0\0\t\0\1\0"...,
> 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 15136
>
> ==> total actions dumped 118
>
> recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
> msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}],
> msg_controllen=0, msg_flags=0}, 0) = 20
>
> --------
>
> We got all 400 in 3 requests. Imagine what we have to deal with
> when we have 2M actions (the improvement is about 10x).
Just because your strace works with the size of 32768 does not mean it
will work in the future.
Please read again my question.
Try to _not_ use 32768 bytes for the recvmsg() sizes, but 4KB
You pack XXX actions until 4KB skb is full.
Then code does :
nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1])
This might fail, then you
goto out_module_put;
Then we are stuck ?
What am I missing ?
^ permalink raw reply
* [PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt
From: Alexander Kochetkov @ 2017-04-19 13:46 UTC (permalink / raw)
To: Florian Fainelli, netdev, linux-kernel; +Cc: Alexander Kochetkov
The problem I fix related to SMSC LAN8710/LAN8720 PHY handled using
interrupts. During power-up cycle the PHY do auto-negotiation, generate
interrupt and set BMSR_ANEGCOMPLETE flag. Interrupt is handled by PHY
state machine but doesn't update link because PHY is in PHY_READY state.
After some time MAC bring up and connect with PHY. It start PHY using
phy_start(). During startup PHY change state to PHY_AN but doesn't
set BMCR_ANRESTART flag due to genphy_config_aneg() doesn't update MII_BMCR
because there no new to advertising. As a result, state machine wait for
interrupt from PHY and nether get "link is up". Because BMSR_ANEGCOMPLETE
already set the patch schedule check link without waiting interrupt.
In case genphy_config_aneg() update MII_BMCR and set BMCR_ANRESTART
flag, BMSR_ANEGCOMPLETE will be cleared and state machine will continue
on auto-negotiation interrupt.
Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
drivers/net/phy/phy.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..da8f03d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1169,6 +1169,18 @@ void phy_state_machine(struct work_struct *work)
if (phydev->irq == PHY_POLL)
queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
PHY_STATE_TIME * HZ);
+
+ /* Re-schedule a PHY state machine to check PHY status because
+ * negotiation already done and aneg interrupt may not be generated.
+ */
+ if (needs_aneg && (phydev->irq > 0) && (phydev->state == PHY_AN)) {
+ err = phy_aneg_done(phydev);
+ if (err > 0)
+ queue_delayed_work(system_power_efficient_wq,
+ &phydev->state_queue, 0);
+ if (err < 0)
+ phy_error(phydev);
+ }
}
/**
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.
From: Andrew Lunn @ 2017-04-19 13:38 UTC (permalink / raw)
To: Roger Quadros
Cc: davem, Florian Fainelli, tony, nsekhar, jsarha, netdev,
linux-omap, linux-kernel
In-Reply-To: <f8624649-3e49-f68c-f364-ee73f1cc514d@ti.com>
On Wed, Apr 19, 2017 at 02:56:48PM +0300, Roger Quadros wrote:
> Hi,
>
> On 19/04/17 14:39, Andrew Lunn wrote:
> > On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
> >> Some boards [1] leave the PHYs at an invalid state
> >> during system power-up or reset thus causing unreliability
> >> issues with the PHY which manifests as PHY not being detected
> >> or link not functional. To fix this, these PHYs need to be RESET
> >> via a GPIO connected to the PHY's RESET pin.
> >>
> >> Some boards have a single GPIO controlling the PHY RESET pin of all
> >> PHYs on the bus whereas some others have separate GPIOs controlling
> >> individual PHY RESETs.
> >>
> >> In both cases, the RESET de-assertion cannot be done in the PHY driver
> >> as the PHY will not probe till its reset is de-asserted.
> >> So do the RESET de-assertion in the MDIO bus driver.
> >>
> >> [1] - am572x-idk, am571x-idk, a437x-idk
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >> drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
> >> drivers/of/of_mdio.c | 4 ++++
> >> include/linux/phy.h | 5 +++++
> >> 3 files changed, 35 insertions(+)
> >
> > Hi Roger
> >
> > Thanks for making this generic.
> >
> > Please add device tree binding documentation. I think that actually
> > means you have to document MDIO in general, since there currently is
> > not a binding document.
>
> OK.
>
> >
> >> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> >> index fa7d51f..25fda2b 100644
> >> --- a/drivers/net/phy/mdio_bus.c
> >> +++ b/drivers/net/phy/mdio_bus.c
> >> @@ -22,8 +22,11 @@
> >> #include <linux/init.h>
> >> #include <linux/delay.h>
> >> #include <linux/device.h>
> >> +#include <linux/gpio.h>
> >> +#include <linux/gpio/consumer.h>
> >> #include <linux/of_device.h>
> >> #include <linux/of_mdio.h>
> >> +#include <linux/of_gpio.h>
> >> #include <linux/netdevice.h>
> >> #include <linux/etherdevice.h>
> >> #include <linux/skbuff.h>
> >> @@ -43,6 +46,8 @@
> >>
> >> #include "mdio-boardinfo.h"
> >>
> >> +#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
> >> +
> >> int mdiobus_register_device(struct mdio_device *mdiodev)
> >> {
> >> if (mdiodev->bus->mdio_map[mdiodev->addr])
> >> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >> {
> >> struct mdio_device *mdiodev;
> >> int i, err;
> >> + struct gpio_desc *gpiod;
> >>
> >> if (NULL == bus || NULL == bus->name ||
> >> NULL == bus->read || NULL == bus->write)
> >> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >> if (bus->reset)
> >> bus->reset(bus);
> >>
> >> + /* de-assert bus level PHY GPIO resets */
> >> + for (i = 0; i < bus->num_reset_gpios; i++) {
> >> + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
> >> + GPIOD_OUT_LOW);
> >> + if (IS_ERR(gpiod)) {
> >> + err = PTR_ERR(gpiod);
> >> + if (err != -ENOENT) {
> >> + pr_err("mii_bus %s couldn't get reset GPIO\n",
> >> + bus->id);
> >> + return err;
> >> + }
> >> + } else {
> >> + gpiod_set_value_cansleep(gpiod, 1);
> >
> >
> >> + if (!bus->reset_delay_us)
> >> + bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
> >
> > Maybe do this once, where you read the device tree property.
>
> I was thinking from point of view that GPIO RESET code should work even without
> device tree. Although I'm not sure if there would be any users or not.
Hi Roger
I don't see how this would work. What would devm_gpiod_get_index()
return? Something from ACPI? But then there would be something
equivalent for getting the delay.
Lets keep it simple and OF only. If there is a real need for something
in addition to OF, it can be added later.
Andrew
^ permalink raw reply
* Re: [patch] socket.7: Document SO_INCOMING_CPU
From: Michael Kerrisk (man-pages) @ 2017-04-19 13:20 UTC (permalink / raw)
To: Francois Saint-Jacques
Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
linux-man-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
Eric Dumazet
In-Reply-To: <63815aac-9c8f-c599-9422-5c312cefc9e8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Ping Eric!
Would you have a chance to review the proposed text below, please.
Thanks,
Michael
On 02/19/2017 09:55 PM, Michael Kerrisk (man-pages) wrote:
> [CC += Eric, so that he might review]
>
> Hello Francois,
>
> On 02/18/2017 05:06 AM, Francois Saint-Jacques wrote:
>> This socket option is undocumented. Applies on the latest version
>> (man-pages-4.09-511).
>>
>> diff --git a/man7/socket.7 b/man7/socket.7
>> index 3efd7a5d8..1a3ffa253 100644
>> --- a/man7/socket.7
>> +++ b/man7/socket.7
>> @@ -490,6 +490,26 @@ flag on a socket
>> operation.
>> Expects an integer boolean flag.
>> .TP
>> +.BR SO_INCOMING_CPU " (getsockopt since Linux 3.19, setsockopt since
>> Linux 4.4)"
>> +.\" getsocktop 2c8c56e15df3d4c2af3d656e44feb18789f75837
>> +.\" setsocktop 70da268b569d32a9fddeea85dc18043de9d89f89
>> +Sets or gets the cpu affinity of a socket. Expects an integer flag.
>> +.sp
>> +.in +4n
>> +.nf
>> +int cpu = 1;
>> +socklen_t len = sizeof(cpu);
>> +setsockopt(fd, SOL_SOCKET, SO_INCOMING_CPU, &cpu, &len);
>> +.fi
>> +.in
>> +.sp
>> +The typical use case is one listener per RX queue, as the associated listener
>> +should only accept flows handled in softirq by the same cpu. This provides
>> +optimal NUMA behavior and keep cpu caches hot.
>> +.TP
>> .B SO_KEEPALIVE
>> Enable sending of keep-alive messages on connection-oriented sockets.
>> Expects an integer boolean flag.
>
> Thank you! Patch applied.
>
> I have tried to enhance the description somewhat. I'm not sure whether
> what I've written is quite correct (or whether it should be further
> extended). Eric, could you please take a look at the following, and let
> me know if anything needs fixing:
>
> SO_INCOMING_CPU (gettable since Linux 3.19, settable since Linux
> 4.4)
> Sets or gets the CPU affinity of a socket. Expects an
> integer flag.
>
> int cpu = 1;
> socklen_t len = sizeof(cpu);
> setsockopt(fd, SOL_SOCKET, SO_INCOMING_CPU, &cpu, &len);
>
> Because all of the packets for a single stream (i.e., all
> packets for the same 4-tuple) arrive on the single RX queue
> that is associated with a particular CPU, the typical use
> case is to employ one listening process per RX queue, with
> the incoming flow being handled by a listener on the same
> CPU that is handling the RX queue. This provides optimal
> NUMA behavior and keeps CPU caches hot.
>
> Cheers,
>
> Michael
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-19 13:13 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <705a4d67-4a81-113f-e22a-0ca0bb6cf1eb@mojatatu.com>
Wed, Apr 19, 2017 at 03:03:59PM CEST, jhs@mojatatu.com wrote:
>On 17-04-19 08:36 AM, Jiri Pirko wrote:
>> Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote:
>> > From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>> > include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
>> > net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
>> > 3 files changed, 53 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> > index cce0613..c7080ec 100644
>> > --- a/include/uapi/linux/rtnetlink.h
>> > +++ b/include/uapi/linux/rtnetlink.h
>> > @@ -674,10 +674,27 @@ struct tcamsg {
>> > unsigned char tca__pad1;
>> > unsigned short tca__pad2;
>> > };
>> > +
>> > +enum {
>> > + TCAA_UNSPEC,
>> > + TCAA_ACT_TAB,
>> > + TCAA_ACT_FLAGS,
>> > + TCAA_ACT_COUNT,
>> > + __TCAA_MAX
>> > +};
>> > +
>> > +#define TCAA_MAX (__TCAA_MAX - 1)
>> > #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> > #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> > -#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>> > -#define TCAA_MAX 1
>> > +#define TCA_ACT_TAB TCAA_ACT_TAB
>>
>> This is mess. What does "TCAA" stand for?
>
>TC Actions Attributes. What would you call it? I could have
>called it TCA_ROOT etc. But maybe a comment to just call it
>TC Actions Attributes would be enough?
TCA_DUMP_X
it is only for dumping. Naming it "attribute" seems weird. Same as if
you have: int variable_something;
>
>> I suggest some more meaningful naming of the enum items and define
>> TCA_ACT_TAB and TCAA_MAX to the new values in order to maintain UAPI
>
>
>Thats what the above does (for UAPI) maintainance, no?
It does it for TCA_ACT_TAB. We need to do it for both TCA_ACT_TAB and TCAA_MAX
>
>> Also, please put X_MAX = __X_MAX - 1 into enum
>
>That is diverting from the norm which defines it outside
>of the enum. A good reason could be: You, Jiri, plan to go and
>cleanup all the netlink stuff which uses this style.
>Or you think we should start a trend which leads us
>to a new clean style.
I would start now. I can take of the follow-up patch to change the rest.
>
>>
>> > +/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
>> > + *
>> > + * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>> > + * actions in a dump. All dump responses will contain the number of actions
>> > + * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
>> > + *
>> > + */
>> > +#define ACT_LARGE_DUMP_ON (1 << 0)
>>
>> Use "BIT(0)"
>>
>
>Same question as before.
Same answer :)
>Are you planning to cleanup the rest of the code which
>follows the same style? example, look at this:
> TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0),
>
>
>> Also use the same prefix as for the enum.
>>
>> + you can have each potential flag as a separate u8 attribute. That is the
>> clearest approach and easily extendable. That's how we do it in devlink
>> for example.
>>
>
>So you are using 8 bits for one flag which requires one bit?
>+ the TLV header? Sounds like overkill.
>Note: We dont need more than 1 or 2 bits for this case.
>Even 32 bits is overkill for what I am doing.
>When do i need to extend a single bit representation?
I don't see any problem adding couple of bytes if it increases cleannes
and easy extendability.
>
>>
>> > struct net *net = sock_net(skb->sk);
>> > - struct nlattr *tca[TCA_ACT_MAX + 1];
>> > + struct nlattr *tca[TCAA_MAX + 1];
>>
>> This is certainly wrong.
>>
>
>Why is it wrong?
Because you use existing TCA_ACT_ attr enum.
>
>>
>> > u32 portid = skb ? NETLINK_CB(skb).portid : 0;
>> > int ret = 0, ovr = 0;
>> >
>> > @@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> > !netlink_capable(skb, CAP_NET_ADMIN))
>> > return -EPERM;
>> >
>> > - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
>> > + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
>>
>> This is certainly wrong.
>>
>
>Same question as above.
Same answer.
>
>
>> > + if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
>> > + goto out_module_put;
>> > + cb->args[1] = 0;
>>
>> Why you need to zero this?
>>
>>
>
>The count is per submitted message - every time we succesfuly send a msg
>to user, we start the recount.
ok
>
>cheers,
>jamal
>
^ permalink raw reply
* Re: [PATCH net-next v4 2/2] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19 13:11 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <20170419125336.GC3357@nanopsycho.orion>
On 17-04-19 08:53 AM, Jiri Pirko wrote:
> Wed, Apr 19, 2017 at 01:57:30PM CEST, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> This adds support for filtering based on time since last used.
>> When we are dumping a large number of actions it is useful to
>> have the option of filtering based on when the action was last
>> used to reduce the amount of data crossing to user space.
>>
>> With this patch the user space app sets the TCAA_ACT_TIME_FILTER
>> attribute with the value in milliseconds with "time of interest
>> since now". The kernel converts this to jiffies and does the
>> filtering comparison matching entries that have seen activity
>> since then and returns them to user space.
>> Old kernels and old tc continue to work in legacy mode since
>> they dont specify this attribute.
>>
>> Some example (we have 400 actions bound to 400 filters); at installation
>> time using hacked tc which sets the time of interest to 120 seconds:
>>
>> prompt$ hackedtc actions ls action gact | grep index | wc -l
>> 400
>>
>> go get some coffee and wait for > 120 seconds and try again:
>>
>> prompt$ hackedtc actions ls action gact | grep index | wc -l
>> 0
>>
>> Lets see a filter bound to one of these actions:
>> ..
>> filter pref 10 u32
>> filter pref 10 u32 fh 800: ht divisor 1
>> filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 2 success 1)
>> match 7f000002/ffffffff at 12 (success 1 )
>> action order 1: gact action pass
>> random type none pass val 0
>> index 23 ref 2 bind 1 installed 1145 sec used 802 sec
>> Action statistics:
>> Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>> ....
>>
>> that coffee took long, no? It was good.
>>
>> Now lets ping -c 1 127.0.0.2, then run the actions again:
>>
>> prompt$ hackedtc actions ls action gact | grep index | wc -l
>> 1
>>
>> More details please:
>>
>> prompt$ hackedtc -s actions ls action gact
>
> I don't see where you pass the time.
>
User space sets the TCAA_ACT_TIME_FILTER value.
Jiri - this is described in the commit log ;->
I have some tc changes which set this value.
In the example below it was 120 seconds.
>
>>
>> action order 0: gact action pass
>> random type none pass val 0
>> index 23 ref 2 bind 1 installed 1270 sec used 30 sec
>> Action statistics:
>> Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>>
>> And the filter?
>>
>> filter pref 10 u32
>> filter pref 10 u32 fh 800: ht divisor 1
>> filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 4 success 2)
>> match 7f000002/ffffffff at 12 (success 2 )
>> action order 1: gact action pass
>> random type none pass val 0
>> index 23 ref 2 bind 1 installed 1324 sec used 84 sec
>> Action statistics:
>> Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> ---
>> include/uapi/linux/rtnetlink.h | 1 +
>> net/sched/act_api.c | 25 +++++++++++++++++++++++--
>> 2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index c7080ec..1b36cc0 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -680,6 +680,7 @@ enum {
>> TCAA_ACT_TAB,
>> TCAA_ACT_FLAGS,
>> TCAA_ACT_COUNT,
>> + TCAA_ACT_TIME_FILTER,
>
> Another use of word "filter" for something else. I believe that we need
> to reduce confusion, not to make it worse.
>
It is a time filter. What do you want to call it?
>
>> __TCAA_MAX
>> };
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 45e1cf7..b03863a 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -84,11 +84,12 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> {
>> int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>> unsigned short act_flags = cb->args[2];
>> + unsigned long jiffy_filter = cb->args[3];
>
> call this "jiffy_since" for example
>
Sure.
>
>> struct nlattr *nest;
>>
>> spin_lock_bh(&hinfo->lock);
>>
>> - s_i = cb->args[0];
>> + s_i = cb->args[4];
>>
>> for (i = 0; i < (hinfo->hmask + 1); i++) {
>> struct hlist_head *head;
>> @@ -101,6 +102,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> if (index < s_i)
>> continue;
>>
>> + if (jiffy_filter &&
>> + time_after(jiffy_filter,
>> + (unsigned long)p->tcfa_tm.lastuse))
>> + continue;
>> +
>> nest = nla_nest_start(skb, n_i);
>> if (nest == NULL)
>> goto nla_put_failure;
>> @@ -118,6 +124,9 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> }
>> }
>> done:
>> + if (index > 0)
>> + cb->args[4] = index + 1;
>> +
>> spin_unlock_bh(&hinfo->lock);
>> if (n_i) {
>> cb->args[0] += n_i;
>
> You don't use "cb->args[0]" anymore. Why do you need this?
You are right - will remove this.
> Just use cb->args[0] instead of 4
>
I can do that too.
>
>> @@ -1000,6 +1009,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>>
>> static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
>> [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
>> + [TCAA_ACT_TIME_FILTER] = { .type = NLA_U32 },
>> };
>>
>> static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> @@ -1090,13 +1100,14 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>> struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
>> struct nlattr *kind = NULL;
>> u32 act_flags = 0;
>> + u32 msecs_filter = 0;
>> + unsigned long jiffy_wanted = 0;
>
> Also, "jiffy_since". Same variable, same name please.
>
np.
>>
>> ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
>> tcaa_policy, NULL);
>> if (ret < 0)
>> return ret;
>>
>> -
>
> This should not be part of this patch.
>
Will remove.
>
>> kind = find_dump_kind(tcaa);
>> if (kind == NULL) {
>> pr_info("tc_dump_action: action bad kind\n");
>> @@ -1110,12 +1121,22 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>> if (tcaa[TCAA_ACT_FLAGS])
>> act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
>>
>> + if (tcaa[TCAA_ACT_TIME_FILTER])
>> + msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
>> +
>> nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
>> cb->nlh->nlmsg_type, sizeof(*t), 0);
>> if (!nlh)
>> goto out_module_put;
>>
>> + if (msecs_filter) {
>> + unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
>> +
>> + jiffy_wanted = jiffies - jiffy_msecs;
>
> you can do just:
> jiffy_wanted = jiffies - msecs_to_jiffies(msecs_filter);
>
> Also, you can put this under "if (tcaa[TCAA_ACT_TIME_FILTER])" so it's
> all in one place.
>
Will do - next cycle run.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-19 13:05 UTC (permalink / raw)
To: Roman Mashak
Cc: Jamal Hadi Salim, davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <85shl4o8te.fsf@mojatatu.com>
Wed, Apr 19, 2017 at 02:55:57PM CEST, mrv@mojatatu.com wrote:
>Jiri Pirko <jiri@resnulli.us> writes:
>
>
>[...]
>
>>>+enum {
>>>+ TCAA_UNSPEC,
>>>+ TCAA_ACT_TAB,
>>>+ TCAA_ACT_FLAGS,
>>>+ TCAA_ACT_COUNT,
>>>+ __TCAA_MAX
>>>+};
>>>+
>>>+#define TCAA_MAX (__TCAA_MAX - 1)
>>> #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>>>-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>>>-#define TCAA_MAX 1
>>>+#define TCA_ACT_TAB TCAA_ACT_TAB
>>
>> This is mess. What does "TCAA" stand for?
>> I suggest some more meaningful naming of the enum items and define
>> TCA_ACT_TAB and TCAA_MAX to the new values in order to maintain UAPI
>> Also, please put X_MAX = __X_MAX - 1 into enum
>>
>
>Notation observed in tc and unfortunately not consistently maintained is
>to have enum with TCA* attributes for instance, followed by define,
>outside of the enum, with __X_MAX -1
I don't have strong opinion on define or in-enum. I like in-enum better.
The rest could be converted later on.
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 13:03 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <20170419123645.GB3357@nanopsycho.orion>
On 17-04-19 08:36 AM, Jiri Pirko wrote:
> Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
>> net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
>> 3 files changed, 53 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index cce0613..c7080ec 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -674,10 +674,27 @@ struct tcamsg {
>> unsigned char tca__pad1;
>> unsigned short tca__pad2;
>> };
>> +
>> +enum {
>> + TCAA_UNSPEC,
>> + TCAA_ACT_TAB,
>> + TCAA_ACT_FLAGS,
>> + TCAA_ACT_COUNT,
>> + __TCAA_MAX
>> +};
>> +
>> +#define TCAA_MAX (__TCAA_MAX - 1)
>> #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> -#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>> -#define TCAA_MAX 1
>> +#define TCA_ACT_TAB TCAA_ACT_TAB
>
> This is mess. What does "TCAA" stand for?
TC Actions Attributes. What would you call it? I could have
called it TCA_ROOT etc. But maybe a comment to just call it
TC Actions Attributes would be enough?
> I suggest some more meaningful naming of the enum items and define
> TCA_ACT_TAB and TCAA_MAX to the new values in order to maintain UAPI
Thats what the above does (for UAPI) maintainance, no?
> Also, please put X_MAX = __X_MAX - 1 into enum
That is diverting from the norm which defines it outside
of the enum. A good reason could be: You, Jiri, plan to go and
cleanup all the netlink stuff which uses this style.
Or you think we should start a trend which leads us
to a new clean style.
>
>> +/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
>> + *
>> + * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>> + * actions in a dump. All dump responses will contain the number of actions
>> + * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
>> + *
>> + */
>> +#define ACT_LARGE_DUMP_ON (1 << 0)
>
> Use "BIT(0)"
>
Same question as before.
Are you planning to cleanup the rest of the code which
follows the same style? example, look at this:
TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0),
> Also use the same prefix as for the enum.
>
> + you can have each potential flag as a separate u8 attribute. That is the
> clearest approach and easily extendable. That's how we do it in devlink
> for example.
>
So you are using 8 bits for one flag which requires one bit?
+ the TLV header? Sounds like overkill.
Note: We dont need more than 1 or 2 bits for this case.
Even 32 bits is overkill for what I am doing.
When do i need to extend a single bit representation?
>
>> struct net *net = sock_net(skb->sk);
>> - struct nlattr *tca[TCA_ACT_MAX + 1];
>> + struct nlattr *tca[TCAA_MAX + 1];
>
> This is certainly wrong.
>
Why is it wrong?
>
>> u32 portid = skb ? NETLINK_CB(skb).portid : 0;
>> int ret = 0, ovr = 0;
>>
>> @@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> !netlink_capable(skb, CAP_NET_ADMIN))
>> return -EPERM;
>>
>> - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
>> + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
>
> This is certainly wrong.
>
Same question as above.
>> + if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
>> + goto out_module_put;
>> + cb->args[1] = 0;
>
> Why you need to zero this?
>
>
The count is per submitted message - every time we succesfuly send a msg
to user, we start the recount.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Roman Mashak @ 2017-04-19 12:55 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jamal Hadi Salim, davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <20170419123645.GB3357@nanopsycho.orion>
Jiri Pirko <jiri@resnulli.us> writes:
[...]
>>+enum {
>>+ TCAA_UNSPEC,
>>+ TCAA_ACT_TAB,
>>+ TCAA_ACT_FLAGS,
>>+ TCAA_ACT_COUNT,
>>+ __TCAA_MAX
>>+};
>>+
>>+#define TCAA_MAX (__TCAA_MAX - 1)
>> #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>>-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>>-#define TCAA_MAX 1
>>+#define TCA_ACT_TAB TCAA_ACT_TAB
>
> This is mess. What does "TCAA" stand for?
> I suggest some more meaningful naming of the enum items and define
> TCA_ACT_TAB and TCAA_MAX to the new values in order to maintain UAPI
> Also, please put X_MAX = __X_MAX - 1 into enum
>
Notation observed in tc and unfortunately not consistently maintained is
to have enum with TCA* attributes for instance, followed by define,
outside of the enum, with __X_MAX -1
[...]
^ permalink raw reply
* Re: [PATCH net-next v4 2/2] net sched actions: add time filter for action dumping
From: Jiri Pirko @ 2017-04-19 12:53 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <1492603050-9318-2-git-send-email-jhs@emojatatu.com>
Wed, Apr 19, 2017 at 01:57:30PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>This adds support for filtering based on time since last used.
>When we are dumping a large number of actions it is useful to
>have the option of filtering based on when the action was last
>used to reduce the amount of data crossing to user space.
>
>With this patch the user space app sets the TCAA_ACT_TIME_FILTER
>attribute with the value in milliseconds with "time of interest
>since now". The kernel converts this to jiffies and does the
>filtering comparison matching entries that have seen activity
>since then and returns them to user space.
>Old kernels and old tc continue to work in legacy mode since
>they dont specify this attribute.
>
>Some example (we have 400 actions bound to 400 filters); at installation
>time using hacked tc which sets the time of interest to 120 seconds:
>
>prompt$ hackedtc actions ls action gact | grep index | wc -l
>400
>
>go get some coffee and wait for > 120 seconds and try again:
>
>prompt$ hackedtc actions ls action gact | grep index | wc -l
>0
>
>Lets see a filter bound to one of these actions:
>..
>filter pref 10 u32
>filter pref 10 u32 fh 800: ht divisor 1
>filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 2 success 1)
> match 7f000002/ffffffff at 12 (success 1 )
> action order 1: gact action pass
> random type none pass val 0
> index 23 ref 2 bind 1 installed 1145 sec used 802 sec
> Action statistics:
> Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
>....
>
>that coffee took long, no? It was good.
>
>Now lets ping -c 1 127.0.0.2, then run the actions again:
>
>prompt$ hackedtc actions ls action gact | grep index | wc -l
>1
>
>More details please:
>
>prompt$ hackedtc -s actions ls action gact
I don't see where you pass the time.
>
> action order 0: gact action pass
> random type none pass val 0
> index 23 ref 2 bind 1 installed 1270 sec used 30 sec
> Action statistics:
> Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
>
>And the filter?
>
>filter pref 10 u32
>filter pref 10 u32 fh 800: ht divisor 1
>filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 4 success 2)
> match 7f000002/ffffffff at 12 (success 2 )
> action order 1: gact action pass
> random type none pass val 0
> index 23 ref 2 bind 1 installed 1324 sec used 84 sec
> Action statistics:
> Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/uapi/linux/rtnetlink.h | 1 +
> net/sched/act_api.c | 25 +++++++++++++++++++++++--
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
>diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>index c7080ec..1b36cc0 100644
>--- a/include/uapi/linux/rtnetlink.h
>+++ b/include/uapi/linux/rtnetlink.h
>@@ -680,6 +680,7 @@ enum {
> TCAA_ACT_TAB,
> TCAA_ACT_FLAGS,
> TCAA_ACT_COUNT,
>+ TCAA_ACT_TIME_FILTER,
Another use of word "filter" for something else. I believe that we need
to reduce confusion, not to make it worse.
> __TCAA_MAX
> };
>
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 45e1cf7..b03863a 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -84,11 +84,12 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> {
> int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
> unsigned short act_flags = cb->args[2];
>+ unsigned long jiffy_filter = cb->args[3];
call this "jiffy_since" for example
> struct nlattr *nest;
>
> spin_lock_bh(&hinfo->lock);
>
>- s_i = cb->args[0];
>+ s_i = cb->args[4];
>
> for (i = 0; i < (hinfo->hmask + 1); i++) {
> struct hlist_head *head;
>@@ -101,6 +102,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> if (index < s_i)
> continue;
>
>+ if (jiffy_filter &&
>+ time_after(jiffy_filter,
>+ (unsigned long)p->tcfa_tm.lastuse))
>+ continue;
>+
> nest = nla_nest_start(skb, n_i);
> if (nest == NULL)
> goto nla_put_failure;
>@@ -118,6 +124,9 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> }
> }
> done:
>+ if (index > 0)
>+ cb->args[4] = index + 1;
>+
> spin_unlock_bh(&hinfo->lock);
> if (n_i) {
> cb->args[0] += n_i;
You don't use "cb->args[0]" anymore. Why do you need this?
Just use cb->args[0] instead of 4
>@@ -1000,6 +1009,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>
> static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
> [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
>+ [TCAA_ACT_TIME_FILTER] = { .type = NLA_U32 },
> };
>
> static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>@@ -1090,13 +1100,14 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
> struct nlattr *kind = NULL;
> u32 act_flags = 0;
>+ u32 msecs_filter = 0;
>+ unsigned long jiffy_wanted = 0;
Also, "jiffy_since". Same variable, same name please.
>
> ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
> tcaa_policy, NULL);
> if (ret < 0)
> return ret;
>
>-
This should not be part of this patch.
> kind = find_dump_kind(tcaa);
> if (kind == NULL) {
> pr_info("tc_dump_action: action bad kind\n");
>@@ -1110,12 +1121,22 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> if (tcaa[TCAA_ACT_FLAGS])
> act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
>
>+ if (tcaa[TCAA_ACT_TIME_FILTER])
>+ msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
>+
> nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> cb->nlh->nlmsg_type, sizeof(*t), 0);
> if (!nlh)
> goto out_module_put;
>
>+ if (msecs_filter) {
>+ unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
>+
>+ jiffy_wanted = jiffies - jiffy_msecs;
you can do just:
jiffy_wanted = jiffies - msecs_to_jiffies(msecs_filter);
Also, you can put this under "if (tcaa[TCAA_ACT_TIME_FILTER])" so it's
all in one place.
>+ }
>+
> cb->args[2] = act_flags;
>+ cb->args[3] = jiffy_wanted;
>
> t = nlmsg_data(nlh);
> t->tca_family = AF_UNSPEC;
>--
>1.9.1
>
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-19 12:36 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <1492603050-9318-1-git-send-email-jhs@emojatatu.com>
Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>When you dump hundreds of thousands of actions, getting only 32 per
>dump batch even when the socket buffer and memory allocations allow
>is inefficient.
>
>With this change, the user will get as many as possibly fitting
>within the given constraints available to the kernel.
>
>A new top level TLV space is introduced. An attribute
>TCAA_ACT_FLAGS is used to carry the flags indicating the user
>is capable of processing these large dumps. Older user space which
>doesnt set this flag doesnt get the large (than 32) batches.
>The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
>actions are put in a single batch. As such user space app knows how long
>to iterate (independent of the type of action being dumped)
>instead of hardcoded maximum of 32.
>
>Some results dumping 1.5M actions, first unpatched tc which the
>kernel doesnt help:
>
>prompt$ time -p tc actions ls action gact | grep index | wc -l
>1500000
>real 1388.43
>user 2.07
>sys 1386.79
>
>Now lets see a patched tc which sets the correct flags when requesting
>a dump:
>
>prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>1500000
>real 178.13
>user 2.02
>sys 176.96
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
> net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
> 3 files changed, 53 insertions(+), 12 deletions(-)
>
>diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>index cce0613..c7080ec 100644
>--- a/include/uapi/linux/rtnetlink.h
>+++ b/include/uapi/linux/rtnetlink.h
>@@ -674,10 +674,27 @@ struct tcamsg {
> unsigned char tca__pad1;
> unsigned short tca__pad2;
> };
>+
>+enum {
>+ TCAA_UNSPEC,
>+ TCAA_ACT_TAB,
>+ TCAA_ACT_FLAGS,
>+ TCAA_ACT_COUNT,
>+ __TCAA_MAX
>+};
>+
>+#define TCAA_MAX (__TCAA_MAX - 1)
> #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>-#define TCAA_MAX 1
>+#define TCA_ACT_TAB TCAA_ACT_TAB
This is mess. What does "TCAA" stand for?
I suggest some more meaningful naming of the enum items and define
TCA_ACT_TAB and TCAA_MAX to the new values in order to maintain UAPI
Also, please put X_MAX = __X_MAX - 1 into enum
>+/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
>+ *
>+ * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>+ * actions in a dump. All dump responses will contain the number of actions
>+ * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
>+ *
>+ */
>+#define ACT_LARGE_DUMP_ON (1 << 0)
Use "BIT(0)"
Also use the same prefix as for the enum.
+ you can have each potential flag as a separate u8 attribute. That is the
clearest approach and easily extendable. That's how we do it in devlink
for example.
>
> /* New extended info filters for IFLA_EXT_MASK */
> #define RTEXT_FILTER_VF (1 << 0)
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 82b1d48..45e1cf7 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> struct netlink_callback *cb)
> {
> int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>+ unsigned short act_flags = cb->args[2];
> struct nlattr *nest;
>
> spin_lock_bh(&hinfo->lock);
>@@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> }
> nla_nest_end(skb, nest);
> n_i++;
>- if (n_i >= TCA_ACT_MAX_PRIO)
>+ if (!(act_flags & ACT_LARGE_DUMP_ON) &&
>+ n_i >= TCA_ACT_MAX_PRIO)
> goto done;
> }
> }
> done:
> spin_unlock_bh(&hinfo->lock);
>- if (n_i)
>+ if (n_i) {
> cb->args[0] += n_i;
>+ if (act_flags & ACT_LARGE_DUMP_ON)
>+ cb->args[1] = n_i;
>+ }
> return n_i;
>
> nla_put_failure:
>@@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> return tcf_add_notify(net, n, &actions, portid);
> }
>
>+static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
>+ [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
>+};
>+
> static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> struct netlink_ext_ack *extack)
> {
> struct net *net = sock_net(skb->sk);
>- struct nlattr *tca[TCA_ACT_MAX + 1];
>+ struct nlattr *tca[TCAA_MAX + 1];
This is certainly wrong.
> u32 portid = skb ? NETLINK_CB(skb).portid : 0;
> int ret = 0, ovr = 0;
>
>@@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> !netlink_capable(skb, CAP_NET_ADMIN))
> return -EPERM;
>
>- ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
>+ ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
This is certainly wrong.
> extack);
> if (ret < 0)
> return ret;
>@@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> return ret;
> }
>
>-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
>+static struct nlattr *find_dump_kind(struct nlattr **nla)
> {
> struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
> struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
>- struct nlattr *nla[TCAA_MAX + 1];
> struct nlattr *kind;
>
>- if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
>- NULL, NULL) < 0)
>- return NULL;
> tb1 = nla[TCA_ACT_TAB];
> if (tb1 == NULL)
> return NULL;
>@@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> struct nlattr *nest;
> struct tc_action_ops *a_o;
> int ret = 0;
>+ struct nlattr *tcaa[TCAA_MAX + 1];
> struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
>- struct nlattr *kind = find_dump_kind(cb->nlh);
>+ struct nlattr *kind = NULL;
>+ u32 act_flags = 0;
>+
>+ ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
>+ tcaa_policy, NULL);
>+ if (ret < 0)
>+ return ret;
>+
>
>+ kind = find_dump_kind(tcaa);
> if (kind == NULL) {
> pr_info("tc_dump_action: action bad kind\n");
> return 0;
>@@ -1093,10 +1107,16 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> if (a_o == NULL)
> return 0;
>
>+ if (tcaa[TCAA_ACT_FLAGS])
>+ act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
>+
> nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> cb->nlh->nlmsg_type, sizeof(*t), 0);
> if (!nlh)
> goto out_module_put;
>+
>+ cb->args[2] = act_flags;
>+
> t = nlmsg_data(nlh);
> t->tca_family = AF_UNSPEC;
> t->tca__pad1 = 0;
>@@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> if (ret > 0) {
> nla_nest_end(skb, nest);
> ret = skb->len;
>+ if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
>+ goto out_module_put;
>+ cb->args[1] = 0;
Why you need to zero this?
> } else
> nlmsg_trim(skb, b);
>
>--
>1.9.1
>
^ permalink raw reply
* Re: XDP question: best API for returning/setting egress port?
From: Daniel Borkmann @ 2017-04-19 12:33 UTC (permalink / raw)
To: Jesper Dangaard Brouer, John Fastabend
Cc: Daniel Borkmann, Alexei Starovoitov, Alexei Starovoitov,
netdev@vger.kernel.org, xdp-newbies@vger.kernel.org
In-Reply-To: <20170419140019.366fb1fb@redhat.com>
On 04/19/2017 02:00 PM, Jesper Dangaard Brouer wrote:
> On Tue, 18 Apr 2017 13:54:45 -0700
> John Fastabend <john.fastabend@gmail.com> wrote:
>> On 17-04-18 12:58 PM, Jesper Dangaard Brouer wrote:
>>>
>>> As I argued in NetConf presentation[1] (from slide #9) we need a port
>>> mapping table (instead of using ifindex'es). Both for supporting
>>> other "port" types than net_devices (think sockets), and for
>>> sandboxing what XDP can bypass.
>>>
>>> I want to create a new XDP action called XDP_REDIRECT, that instruct
>>> XDP to send the xdp_buff to another "port" (get translated into a
>>> net_device, or something else depending on internal port type).
>>>
>>> Looking at the userspace/eBPF interface, I'm wondering what is the
>>> best API for "returning" this port number from eBPF?
>>>
>>> The options I see is:
>>>
>>> 1) Split-up the u32 action code, and e.g let the high-16-bit be the
>>> port number and lower-16bit the (existing) action verdict.
>>>
>>> Pros: Simple API
>>> Cons: Number of ports limited to 64K
>>>
>>> 2) Extend both xdp_buff + xdp_md to contain a (u32) port number, allow
>>> eBPF to update xdp_md->port.
>>>
>>> Pros: Larger number of ports.
>>> Cons: This require some ebpf translation steps between xdp_buff <-> xdp_md.
>>> (see xdp_convert_ctx_access)
>>>
>>> 3) Extend only xdp_buff and create bpf_helper that set port in xdp_buff.
>>>
>>> Pros: Hides impl details, and allows helper to give eBPF code feedback
>>> (on e.g. if port doesn't exist any longer)
>>> Cons: Helper function call likely slower?
>>
>> How about doing this the same way redirect is done in the tc case? I have this
>> patch under test,
>>
>> https://github.com/jrfastab/linux/commit/e78f5425d5e3c305b4170ddd85c61c2e15359fee
>
> I have been looking at this approach, which is close to option #3 above.
>
> The problem with your implementation that you use a per-cpu store.
> This creates the problem of storing state between packets. First packet
> can call helper bpf_xdp_redirect() setting an ifindex, but program can
> still return XDP_PASS. Next packet can call XDP_REDIRECT and use the
> ifindex set from the first packet. IMHO this is a problematic API to
> expose.
>
> I do see that the TC interface that uses the same approach, via helper
> bpf_redirect(). Maybe it have the same API problem? Looking at
> sch_handle_ingress() I don't see this is handled (e.g. by always
> clearing this_cpu_ptr(redirect_info)->ifindex = 0).
It's cleared in {skb,xdp}_do_redirect() right after fetching the
ifindex. I think this approach is just fine. The example described
above is a misuse of the API by a buggy program calling bpf_xdp_redirect()
and returning XDP_PASS while another time it returns XDP_REDIRECT
without the bpf_xdp_redirect() helper, sounds very exotic, but it's
as buggy as, say, a program doing the csum update wrong, a program
writing the wrong data to the packet, doing adjust head on the wrong
header offset, jumping into the wrong tail call entry and other things.
I think encoding this into an action code is rather limiting, f.e.
where would we place a flags argument if needed in future? Would
that mean, we need a XDP_REDIRECT2 return code that also allows for
encoding flags?
Thanks,
Daniel
^ permalink raw reply
* Re: XDP question: best API for returning/setting egress port?
From: Hannes Frederic Sowa @ 2017-04-19 12:25 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
Alexei Starovoitov
Cc: netdev@vger.kernel.org, xdp-newbies@vger.kernel.org,
John Fastabend
In-Reply-To: <20170418215856.5fda7127@redhat.com>
Hi,
On 18.04.2017 21:58, Jesper Dangaard Brouer wrote:
>
> As I argued in NetConf presentation[1] (from slide #9) we need a port
> mapping table (instead of using ifindex'es). Both for supporting
> other "port" types than net_devices (think sockets), and for
> sandboxing what XDP can bypass.
>
> I want to create a new XDP action called XDP_REDIRECT, that instruct
> XDP to send the xdp_buff to another "port" (get translated into a
> net_device, or something else depending on internal port type).
>
> Looking at the userspace/eBPF interface, I'm wondering what is the
> best API for "returning" this port number from eBPF?
>
> The options I see is:
>
> 1) Split-up the u32 action code, and e.g let the high-16-bit be the
> port number and lower-16bit the (existing) action verdict.
>
> Pros: Simple API
> Cons: Number of ports limited to 64K
>
> 2) Extend both xdp_buff + xdp_md to contain a (u32) port number, allow
> eBPF to update xdp_md->port.
>
> Pros: Larger number of ports.
> Cons: This require some ebpf translation steps between xdp_buff <-> xdp_md.
> (see xdp_convert_ctx_access)
>
> 3) Extend only xdp_buff and create bpf_helper that set port in xdp_buff.
>
> Pros: Hides impl details, and allows helper to give eBPF code feedback
> (on e.g. if port doesn't exist any longer)
> Cons: Helper function call likely slower?
>
>
> (Cc'ed xdp-newbies as end-users might have an opinion on UAPI?)
I am not sure how the socket interface should look like, it seems to be
too far away to me right now.
Regarding having stable ifindexes, I wonder if we could do something:
int ifindexes_in_use_by_ebpf_program[] __section("ifindex") = {
1,2,3,8,9,10 };
and we can make sure that the ifindexes automatically stay stable for
the lifetime while the ebpf program is loaded?
Bye,
Hannes
^ permalink raw reply
* Re: XDP question: best API for returning/setting egress port?
From: Jesper Dangaard Brouer @ 2017-04-19 12:00 UTC (permalink / raw)
To: John Fastabend
Cc: Daniel Borkmann, Alexei Starovoitov, Alexei Starovoitov,
netdev@vger.kernel.org, xdp-newbies@vger.kernel.org, brouer
In-Reply-To: <58F67D15.3050308@gmail.com>
On Tue, 18 Apr 2017 13:54:45 -0700
John Fastabend <john.fastabend@gmail.com> wrote:
> On 17-04-18 12:58 PM, Jesper Dangaard Brouer wrote:
> >
> > As I argued in NetConf presentation[1] (from slide #9) we need a port
> > mapping table (instead of using ifindex'es). Both for supporting
> > other "port" types than net_devices (think sockets), and for
> > sandboxing what XDP can bypass.
> >
> > I want to create a new XDP action called XDP_REDIRECT, that instruct
> > XDP to send the xdp_buff to another "port" (get translated into a
> > net_device, or something else depending on internal port type).
> >
> > Looking at the userspace/eBPF interface, I'm wondering what is the
> > best API for "returning" this port number from eBPF?
> >
> > The options I see is:
> >
> > 1) Split-up the u32 action code, and e.g let the high-16-bit be the
> > port number and lower-16bit the (existing) action verdict.
> >
> > Pros: Simple API
> > Cons: Number of ports limited to 64K
> >
> > 2) Extend both xdp_buff + xdp_md to contain a (u32) port number, allow
> > eBPF to update xdp_md->port.
> >
> > Pros: Larger number of ports.
> > Cons: This require some ebpf translation steps between xdp_buff <-> xdp_md.
> > (see xdp_convert_ctx_access)
> >
> > 3) Extend only xdp_buff and create bpf_helper that set port in xdp_buff.
> >
> > Pros: Hides impl details, and allows helper to give eBPF code feedback
> > (on e.g. if port doesn't exist any longer)
> > Cons: Helper function call likely slower?
> >
> >
>
> How about doing this the same way redirect is done in the tc case? I have this
> patch under test,
>
> https://github.com/jrfastab/linux/commit/e78f5425d5e3c305b4170ddd85c61c2e15359fee
I have been looking at this approach, which is close to option #3 above.
The problem with your implementation that you use a per-cpu store.
This creates the problem of storing state between packets. First packet
can call helper bpf_xdp_redirect() setting an ifindex, but program can
still return XDP_PASS. Next packet can call XDP_REDIRECT and use the
ifindex set from the first packet. IMHO this is a problematic API to
expose.
I do see that the TC interface that uses the same approach, via helper
bpf_redirect(). Maybe it have the same API problem? Looking at
sch_handle_ingress() I don't see this is handled (e.g. by always
clearing this_cpu_ptr(redirect_info)->ifindex = 0).
> that should give you some idea. It just needs a port mapping table in the
> bpf_tx_xdp() call.
I'll take a closer look. I don't think we need the per-cpu-store
approach for XDP, as we might as well store the port info in xdp_buff,
or return it directly option #1.
(TC redirect need the per-cpu-store to avoid extending the SKB).
> > (Cc'ed xdp-newbies as end-users might have an opinion on UAPI?)
I would still like people to comment on the above options?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH net-next v4 2/2] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19 11:57 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
In-Reply-To: <1492603050-9318-1-git-send-email-jhs@emojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
This adds support for filtering based on time since last used.
When we are dumping a large number of actions it is useful to
have the option of filtering based on when the action was last
used to reduce the amount of data crossing to user space.
With this patch the user space app sets the TCAA_ACT_TIME_FILTER
attribute with the value in milliseconds with "time of interest
since now". The kernel converts this to jiffies and does the
filtering comparison matching entries that have seen activity
since then and returns them to user space.
Old kernels and old tc continue to work in legacy mode since
they dont specify this attribute.
Some example (we have 400 actions bound to 400 filters); at installation
time using hacked tc which sets the time of interest to 120 seconds:
prompt$ hackedtc actions ls action gact | grep index | wc -l
400
go get some coffee and wait for > 120 seconds and try again:
prompt$ hackedtc actions ls action gact | grep index | wc -l
0
Lets see a filter bound to one of these actions:
..
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 2 success 1)
match 7f000002/ffffffff at 12 (success 1 )
action order 1: gact action pass
random type none pass val 0
index 23 ref 2 bind 1 installed 1145 sec used 802 sec
Action statistics:
Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
....
that coffee took long, no? It was good.
Now lets ping -c 1 127.0.0.2, then run the actions again:
prompt$ hackedtc actions ls action gact | grep index | wc -l
1
More details please:
prompt$ hackedtc -s actions ls action gact
action order 0: gact action pass
random type none pass val 0
index 23 ref 2 bind 1 installed 1270 sec used 30 sec
Action statistics:
Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
And the filter?
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 4 success 2)
match 7f000002/ffffffff at 12 (success 2 )
action order 1: gact action pass
random type none pass val 0
index 23 ref 2 bind 1 installed 1324 sec used 84 sec
Action statistics:
Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
include/uapi/linux/rtnetlink.h | 1 +
net/sched/act_api.c | 25 +++++++++++++++++++++++--
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index c7080ec..1b36cc0 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -680,6 +680,7 @@ enum {
TCAA_ACT_TAB,
TCAA_ACT_FLAGS,
TCAA_ACT_COUNT,
+ TCAA_ACT_TIME_FILTER,
__TCAA_MAX
};
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 45e1cf7..b03863a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -84,11 +84,12 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
{
int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
unsigned short act_flags = cb->args[2];
+ unsigned long jiffy_filter = cb->args[3];
struct nlattr *nest;
spin_lock_bh(&hinfo->lock);
- s_i = cb->args[0];
+ s_i = cb->args[4];
for (i = 0; i < (hinfo->hmask + 1); i++) {
struct hlist_head *head;
@@ -101,6 +102,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
if (index < s_i)
continue;
+ if (jiffy_filter &&
+ time_after(jiffy_filter,
+ (unsigned long)p->tcfa_tm.lastuse))
+ continue;
+
nest = nla_nest_start(skb, n_i);
if (nest == NULL)
goto nla_put_failure;
@@ -118,6 +124,9 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
}
}
done:
+ if (index > 0)
+ cb->args[4] = index + 1;
+
spin_unlock_bh(&hinfo->lock);
if (n_i) {
cb->args[0] += n_i;
@@ -1000,6 +1009,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
[TCAA_ACT_FLAGS] = { .type = NLA_U32 },
+ [TCAA_ACT_TIME_FILTER] = { .type = NLA_U32 },
};
static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
@@ -1090,13 +1100,14 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
struct nlattr *kind = NULL;
u32 act_flags = 0;
+ u32 msecs_filter = 0;
+ unsigned long jiffy_wanted = 0;
ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
tcaa_policy, NULL);
if (ret < 0)
return ret;
-
kind = find_dump_kind(tcaa);
if (kind == NULL) {
pr_info("tc_dump_action: action bad kind\n");
@@ -1110,12 +1121,22 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (tcaa[TCAA_ACT_FLAGS])
act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
+ if (tcaa[TCAA_ACT_TIME_FILTER])
+ msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
+
nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
cb->nlh->nlmsg_type, sizeof(*t), 0);
if (!nlh)
goto out_module_put;
+ if (msecs_filter) {
+ unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
+
+ jiffy_wanted = jiffies - jiffy_msecs;
+ }
+
cb->args[2] = act_flags;
+ cb->args[3] = jiffy_wanted;
t = nlmsg_data(nlh);
t->tca_family = AF_UNSPEC;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 11:57 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
When you dump hundreds of thousands of actions, getting only 32 per
dump batch even when the socket buffer and memory allocations allow
is inefficient.
With this change, the user will get as many as possibly fitting
within the given constraints available to the kernel.
A new top level TLV space is introduced. An attribute
TCAA_ACT_FLAGS is used to carry the flags indicating the user
is capable of processing these large dumps. Older user space which
doesnt set this flag doesnt get the large (than 32) batches.
The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
actions are put in a single batch. As such user space app knows how long
to iterate (independent of the type of action being dumped)
instead of hardcoded maximum of 32.
Some results dumping 1.5M actions, first unpatched tc which the
kernel doesnt help:
prompt$ time -p tc actions ls action gact | grep index | wc -l
1500000
real 1388.43
user 2.07
sys 1386.79
Now lets see a patched tc which sets the correct flags when requesting
a dump:
prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
1500000
real 178.13
user 2.02
sys 176.96
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
3 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index cce0613..c7080ec 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -674,10 +674,27 @@ struct tcamsg {
unsigned char tca__pad1;
unsigned short tca__pad2;
};
+
+enum {
+ TCAA_UNSPEC,
+ TCAA_ACT_TAB,
+ TCAA_ACT_FLAGS,
+ TCAA_ACT_COUNT,
+ __TCAA_MAX
+};
+
+#define TCAA_MAX (__TCAA_MAX - 1)
#define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
#define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
-#define TCAA_MAX 1
+#define TCA_ACT_TAB TCAA_ACT_TAB
+/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
+ *
+ * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
+ * actions in a dump. All dump responses will contain the number of actions
+ * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
+ *
+ */
+#define ACT_LARGE_DUMP_ON (1 << 0)
/* New extended info filters for IFLA_EXT_MASK */
#define RTEXT_FILTER_VF (1 << 0)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48..45e1cf7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
struct netlink_callback *cb)
{
int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
+ unsigned short act_flags = cb->args[2];
struct nlattr *nest;
spin_lock_bh(&hinfo->lock);
@@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
}
nla_nest_end(skb, nest);
n_i++;
- if (n_i >= TCA_ACT_MAX_PRIO)
+ if (!(act_flags & ACT_LARGE_DUMP_ON) &&
+ n_i >= TCA_ACT_MAX_PRIO)
goto done;
}
}
done:
spin_unlock_bh(&hinfo->lock);
- if (n_i)
+ if (n_i) {
cb->args[0] += n_i;
+ if (act_flags & ACT_LARGE_DUMP_ON)
+ cb->args[1] = n_i;
+ }
return n_i;
nla_put_failure:
@@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
return tcf_add_notify(net, n, &actions, portid);
}
+static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
+ [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
+};
+
static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
struct netlink_ext_ack *extack)
{
struct net *net = sock_net(skb->sk);
- struct nlattr *tca[TCA_ACT_MAX + 1];
+ struct nlattr *tca[TCAA_MAX + 1];
u32 portid = skb ? NETLINK_CB(skb).portid : 0;
int ret = 0, ovr = 0;
@@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
!netlink_capable(skb, CAP_NET_ADMIN))
return -EPERM;
- ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+ ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
extack);
if (ret < 0)
return ret;
@@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
return ret;
}
-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
+static struct nlattr *find_dump_kind(struct nlattr **nla)
{
struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
- struct nlattr *nla[TCAA_MAX + 1];
struct nlattr *kind;
- if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
- NULL, NULL) < 0)
- return NULL;
tb1 = nla[TCA_ACT_TAB];
if (tb1 == NULL)
return NULL;
@@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
struct nlattr *nest;
struct tc_action_ops *a_o;
int ret = 0;
+ struct nlattr *tcaa[TCAA_MAX + 1];
struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
- struct nlattr *kind = find_dump_kind(cb->nlh);
+ struct nlattr *kind = NULL;
+ u32 act_flags = 0;
+
+ ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
+ tcaa_policy, NULL);
+ if (ret < 0)
+ return ret;
+
+ kind = find_dump_kind(tcaa);
if (kind == NULL) {
pr_info("tc_dump_action: action bad kind\n");
return 0;
@@ -1093,10 +1107,16 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (a_o == NULL)
return 0;
+ if (tcaa[TCAA_ACT_FLAGS])
+ act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
+
nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
cb->nlh->nlmsg_type, sizeof(*t), 0);
if (!nlh)
goto out_module_put;
+
+ cb->args[2] = act_flags;
+
t = nlmsg_data(nlh);
t->tca_family = AF_UNSPEC;
t->tca__pad1 = 0;
@@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (ret > 0) {
nla_nest_end(skb, nest);
ret = skb->len;
+ if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
+ goto out_module_put;
+ cb->args[1] = 0;
} else
nlmsg_trim(skb, b);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.
From: Roger Quadros @ 2017-04-19 11:56 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, Florian Fainelli, tony, nsekhar, jsarha, netdev,
linux-omap, linux-kernel
In-Reply-To: <20170419113951.GA31908@lunn.ch>
Hi,
On 19/04/17 14:39, Andrew Lunn wrote:
> On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
>> drivers/of/of_mdio.c | 4 ++++
>> include/linux/phy.h | 5 +++++
>> 3 files changed, 35 insertions(+)
>
> Hi Roger
>
> Thanks for making this generic.
>
> Please add device tree binding documentation. I think that actually
> means you have to document MDIO in general, since there currently is
> not a binding document.
OK.
>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index fa7d51f..25fda2b 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -22,8 +22,11 @@
>> #include <linux/init.h>
>> #include <linux/delay.h>
>> #include <linux/device.h>
>> +#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>> #include <linux/of_device.h>
>> #include <linux/of_mdio.h>
>> +#include <linux/of_gpio.h>
>> #include <linux/netdevice.h>
>> #include <linux/etherdevice.h>
>> #include <linux/skbuff.h>
>> @@ -43,6 +46,8 @@
>>
>> #include "mdio-boardinfo.h"
>>
>> +#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
>> +
>> int mdiobus_register_device(struct mdio_device *mdiodev)
>> {
>> if (mdiodev->bus->mdio_map[mdiodev->addr])
>> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>> {
>> struct mdio_device *mdiodev;
>> int i, err;
>> + struct gpio_desc *gpiod;
>>
>> if (NULL == bus || NULL == bus->name ||
>> NULL == bus->read || NULL == bus->write)
>> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>> if (bus->reset)
>> bus->reset(bus);
>>
>> + /* de-assert bus level PHY GPIO resets */
>> + for (i = 0; i < bus->num_reset_gpios; i++) {
>> + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(gpiod)) {
>> + err = PTR_ERR(gpiod);
>> + if (err != -ENOENT) {
>> + pr_err("mii_bus %s couldn't get reset GPIO\n",
>> + bus->id);
>> + return err;
>> + }
>> + } else {
>> + gpiod_set_value_cansleep(gpiod, 1);
>
>
>> + if (!bus->reset_delay_us)
>> + bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
>
> Maybe do this once, where you read the device tree property.
I was thinking from point of view that GPIO RESET code should work even without
device tree. Although I'm not sure if there would be any users or not.
>
>
>> + udelay(bus->reset_delay_us);
>> + gpiod_set_value_cansleep(gpiod, 0);
>> + }
>> + }
>> +
>> for (i = 0; i < PHY_MAX_ADDR; i++) {
>> if ((bus->phy_mask & (1 << i)) == 0) {
>> struct phy_device *phydev;
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 0b29798..83a62e4 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>>
>> mdio->dev.of_node = np;
>>
>> + /* Get bus level PHY reset GPIO details */
>> + of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
>
> If the property does not exist, it is guaranteed mdio->reset_delay_us
> is not changed. So you can set it to the default value before, then do
> this call.
>
> Andrew
>
cheers,
-roger
^ permalink raw reply
* Re: [PATCH] dp83640: don't recieve time stamps twice
From: Richard Cochran @ 2017-04-19 11:54 UTC (permalink / raw)
To: Sørensen, Stefan
Cc: netdev@vger.kernel.org, dan.carpenter@oracle.com,
f.fainelli@gmail.com, andrew@lunn.ch,
kernel-janitors@vger.kernel.org
In-Reply-To: <1492601312.3108.3.camel@spectralink.com>
On Wed, Apr 19, 2017 at 11:28:34AM +0000, Sørensen, Stefan wrote:
> Here we still hold rx_lock while during the callback, wouldn't it be
> beneficial to release that first?
Yes, your are right.
Do you want to post a fix? If not, I will, but later on this week.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH net-next v3 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 11:53 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong
In-Reply-To: <1492602564-9100-1-git-send-email-jhs@emojatatu.com>
On 17-04-19 07:49 AM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> When you dump hundreds of thousands of actions, getting only 32 per
> dump batch even when the socket buffer and memory allocations allow
> is inefficient.
>
> With this change, the user will get as many as possibly fitting
> within the given constraints available to the kernel.
>
> A new top level TLV space is introduced. An attribute
> TCAA_ACT_FLAGS is used to carry the flags indicating the user
> is capable of processing these large dumps. Older user space which
> doesnt set this flag doesnt get the large (than 32) batches.
> The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
> actions are put in a single batch. As such user space app knows how long
> to iterate (independent of the type of action being dumped)
> instead of hardcoded maximum of 32.
>
> Some results dumping 1.5M actions, first unpatched tc which the
> kernel doesnt help:
>
> prompt$ time -p tc actions ls action gact | grep index | wc -l
> 1500000
> real 1388.43
> user 2.07
> sys 1386.79
>
> Now lets see a patched tc which sets the correct flags when requesting
> a dump:
>
> prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
> 1500000
> real 178.13
> user 2.02
> sys 176.96
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
> MAINTAINERS | 1 +
> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
> net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
> 3 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b4b828..94eadbc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12198,6 +12198,7 @@ F: kernel/taskstats.c
>
> TC subsystem
> M: Jamal Hadi Salim <jhs@mojatatu.com>
> +M: Jamal Hadi Salim <jhs@mojatatu.com>
> L: netdev@vger.kernel.org
> S: Maintained
> F: include/net/pkt_cls.h
Grrr. I did it again. Sending v4 shortly
cheers,
jamal
^ permalink raw reply
* [PATCH net-next v3 2/2] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19 11:49 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
In-Reply-To: <1492602564-9100-1-git-send-email-jhs@emojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
This adds support for filtering based on time since last used.
When we are dumping a large number of actions it is useful to
have the option of filtering based on when the action was last
used to reduce the amount of data crossing to user space.
With this patch the user space app sets the TCAA_ACT_TIME_FILTER
attribute with the value in milliseconds with "time of interest
since now". The kernel converts this to jiffies and does the
filtering comparison matching entries that have seen activity
since then and returns them to user space.
Old kernels and old tc continue to work in legacy mode since
they dont specify this attribute.
Some example (we have 400 actions bound to 400 filters); at installation
time using hacked tc which sets the time of interest to 120 seconds:
prompt$ hackedtc actions ls action gact | grep index | wc -l
400
go get some coffee and wait for > 120 seconds and try again:
prompt$ hackedtc actions ls action gact | grep index | wc -l
0
Lets see a filter bound to one of these actions:
..
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 2 success 1)
match 7f000002/ffffffff at 12 (success 1 )
action order 1: gact action pass
random type none pass val 0
index 23 ref 2 bind 1 installed 1145 sec used 802 sec
Action statistics:
Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
....
that coffee took long, no? It was good.
Now lets ping -c 1 127.0.0.2, then run the actions again:
prompt$ hackedtc actions ls action gact | grep index | wc -l
1
More details please:
prompt$ hackedtc -s actions ls action gact
action order 0: gact action pass
random type none pass val 0
index 23 ref 2 bind 1 installed 1270 sec used 30 sec
Action statistics:
Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
And the filter?
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 4 success 2)
match 7f000002/ffffffff at 12 (success 2 )
action order 1: gact action pass
random type none pass val 0
index 23 ref 2 bind 1 installed 1324 sec used 84 sec
Action statistics:
Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
include/uapi/linux/rtnetlink.h | 1 +
net/sched/act_api.c | 25 +++++++++++++++++++++++--
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index c7080ec..1b36cc0 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -680,6 +680,7 @@ enum {
TCAA_ACT_TAB,
TCAA_ACT_FLAGS,
TCAA_ACT_COUNT,
+ TCAA_ACT_TIME_FILTER,
__TCAA_MAX
};
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 45e1cf7..b03863a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -84,11 +84,12 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
{
int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
unsigned short act_flags = cb->args[2];
+ unsigned long jiffy_filter = cb->args[3];
struct nlattr *nest;
spin_lock_bh(&hinfo->lock);
- s_i = cb->args[0];
+ s_i = cb->args[4];
for (i = 0; i < (hinfo->hmask + 1); i++) {
struct hlist_head *head;
@@ -101,6 +102,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
if (index < s_i)
continue;
+ if (jiffy_filter &&
+ time_after(jiffy_filter,
+ (unsigned long)p->tcfa_tm.lastuse))
+ continue;
+
nest = nla_nest_start(skb, n_i);
if (nest == NULL)
goto nla_put_failure;
@@ -118,6 +124,9 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
}
}
done:
+ if (index > 0)
+ cb->args[4] = index + 1;
+
spin_unlock_bh(&hinfo->lock);
if (n_i) {
cb->args[0] += n_i;
@@ -1000,6 +1009,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
[TCAA_ACT_FLAGS] = { .type = NLA_U32 },
+ [TCAA_ACT_TIME_FILTER] = { .type = NLA_U32 },
};
static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
@@ -1090,13 +1100,14 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
struct nlattr *kind = NULL;
u32 act_flags = 0;
+ u32 msecs_filter = 0;
+ unsigned long jiffy_wanted = 0;
ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
tcaa_policy, NULL);
if (ret < 0)
return ret;
-
kind = find_dump_kind(tcaa);
if (kind == NULL) {
pr_info("tc_dump_action: action bad kind\n");
@@ -1110,12 +1121,22 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (tcaa[TCAA_ACT_FLAGS])
act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
+ if (tcaa[TCAA_ACT_TIME_FILTER])
+ msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
+
nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
cb->nlh->nlmsg_type, sizeof(*t), 0);
if (!nlh)
goto out_module_put;
+ if (msecs_filter) {
+ unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
+
+ jiffy_wanted = jiffies - jiffy_msecs;
+ }
+
cb->args[2] = act_flags;
+ cb->args[3] = jiffy_wanted;
t = nlmsg_data(nlh);
t->tca_family = AF_UNSPEC;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next v3 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 11:49 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
When you dump hundreds of thousands of actions, getting only 32 per
dump batch even when the socket buffer and memory allocations allow
is inefficient.
With this change, the user will get as many as possibly fitting
within the given constraints available to the kernel.
A new top level TLV space is introduced. An attribute
TCAA_ACT_FLAGS is used to carry the flags indicating the user
is capable of processing these large dumps. Older user space which
doesnt set this flag doesnt get the large (than 32) batches.
The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
actions are put in a single batch. As such user space app knows how long
to iterate (independent of the type of action being dumped)
instead of hardcoded maximum of 32.
Some results dumping 1.5M actions, first unpatched tc which the
kernel doesnt help:
prompt$ time -p tc actions ls action gact | grep index | wc -l
1500000
real 1388.43
user 2.07
sys 1386.79
Now lets see a patched tc which sets the correct flags when requesting
a dump:
prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
1500000
real 178.13
user 2.02
sys 176.96
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
MAINTAINERS | 1 +
include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
3 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b4b828..94eadbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12198,6 +12198,7 @@ F: kernel/taskstats.c
TC subsystem
M: Jamal Hadi Salim <jhs@mojatatu.com>
+M: Jamal Hadi Salim <jhs@mojatatu.com>
L: netdev@vger.kernel.org
S: Maintained
F: include/net/pkt_cls.h
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index cce0613..c7080ec 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -674,10 +674,27 @@ struct tcamsg {
unsigned char tca__pad1;
unsigned short tca__pad2;
};
+
+enum {
+ TCAA_UNSPEC,
+ TCAA_ACT_TAB,
+ TCAA_ACT_FLAGS,
+ TCAA_ACT_COUNT,
+ __TCAA_MAX
+};
+
+#define TCAA_MAX (__TCAA_MAX - 1)
#define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
#define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
-#define TCAA_MAX 1
+#define TCA_ACT_TAB TCAA_ACT_TAB
+/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
+ *
+ * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
+ * actions in a dump. All dump responses will contain the number of actions
+ * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
+ *
+ */
+#define ACT_LARGE_DUMP_ON (1 << 0)
/* New extended info filters for IFLA_EXT_MASK */
#define RTEXT_FILTER_VF (1 << 0)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48..45e1cf7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
struct netlink_callback *cb)
{
int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
+ unsigned short act_flags = cb->args[2];
struct nlattr *nest;
spin_lock_bh(&hinfo->lock);
@@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
}
nla_nest_end(skb, nest);
n_i++;
- if (n_i >= TCA_ACT_MAX_PRIO)
+ if (!(act_flags & ACT_LARGE_DUMP_ON) &&
+ n_i >= TCA_ACT_MAX_PRIO)
goto done;
}
}
done:
spin_unlock_bh(&hinfo->lock);
- if (n_i)
+ if (n_i) {
cb->args[0] += n_i;
+ if (act_flags & ACT_LARGE_DUMP_ON)
+ cb->args[1] = n_i;
+ }
return n_i;
nla_put_failure:
@@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
return tcf_add_notify(net, n, &actions, portid);
}
+static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
+ [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
+};
+
static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
struct netlink_ext_ack *extack)
{
struct net *net = sock_net(skb->sk);
- struct nlattr *tca[TCA_ACT_MAX + 1];
+ struct nlattr *tca[TCAA_MAX + 1];
u32 portid = skb ? NETLINK_CB(skb).portid : 0;
int ret = 0, ovr = 0;
@@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
!netlink_capable(skb, CAP_NET_ADMIN))
return -EPERM;
- ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+ ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
extack);
if (ret < 0)
return ret;
@@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
return ret;
}
-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
+static struct nlattr *find_dump_kind(struct nlattr **nla)
{
struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
- struct nlattr *nla[TCAA_MAX + 1];
struct nlattr *kind;
- if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
- NULL, NULL) < 0)
- return NULL;
tb1 = nla[TCA_ACT_TAB];
if (tb1 == NULL)
return NULL;
@@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
struct nlattr *nest;
struct tc_action_ops *a_o;
int ret = 0;
+ struct nlattr *tcaa[TCAA_MAX + 1];
struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
- struct nlattr *kind = find_dump_kind(cb->nlh);
+ struct nlattr *kind = NULL;
+ u32 act_flags = 0;
+
+ ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
+ tcaa_policy, NULL);
+ if (ret < 0)
+ return ret;
+
+ kind = find_dump_kind(tcaa);
if (kind == NULL) {
pr_info("tc_dump_action: action bad kind\n");
return 0;
@@ -1093,10 +1107,16 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (a_o == NULL)
return 0;
+ if (tcaa[TCAA_ACT_FLAGS])
+ act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
+
nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
cb->nlh->nlmsg_type, sizeof(*t), 0);
if (!nlh)
goto out_module_put;
+
+ cb->args[2] = act_flags;
+
t = nlmsg_data(nlh);
t->tca_family = AF_UNSPEC;
t->tca__pad1 = 0;
@@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (ret > 0) {
nla_nest_end(skb, nest);
ret = skb->len;
+ if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
+ goto out_module_put;
+ cb->args[1] = 0;
} else
nlmsg_trim(skb, b);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.
From: Andrew Lunn @ 2017-04-19 11:39 UTC (permalink / raw)
To: Roger Quadros
Cc: davem, Florian Fainelli, tony, nsekhar, jsarha, netdev,
linux-omap, linux-kernel
In-Reply-To: <64d6494d-41d2-0faf-a434-057f796637fe@ti.com>
On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
>
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
>
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
>
> [1] - am572x-idk, am571x-idk, a437x-idk
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
> drivers/of/of_mdio.c | 4 ++++
> include/linux/phy.h | 5 +++++
> 3 files changed, 35 insertions(+)
Hi Roger
Thanks for making this generic.
Please add device tree binding documentation. I think that actually
means you have to document MDIO in general, since there currently is
not a binding document.
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index fa7d51f..25fda2b 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -22,8 +22,11 @@
> #include <linux/init.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/of_device.h>
> #include <linux/of_mdio.h>
> +#include <linux/of_gpio.h>
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> #include <linux/skbuff.h>
> @@ -43,6 +46,8 @@
>
> #include "mdio-boardinfo.h"
>
> +#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
> +
> int mdiobus_register_device(struct mdio_device *mdiodev)
> {
> if (mdiodev->bus->mdio_map[mdiodev->addr])
> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> {
> struct mdio_device *mdiodev;
> int i, err;
> + struct gpio_desc *gpiod;
>
> if (NULL == bus || NULL == bus->name ||
> NULL == bus->read || NULL == bus->write)
> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> if (bus->reset)
> bus->reset(bus);
>
> + /* de-assert bus level PHY GPIO resets */
> + for (i = 0; i < bus->num_reset_gpios; i++) {
> + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
> + GPIOD_OUT_LOW);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + if (err != -ENOENT) {
> + pr_err("mii_bus %s couldn't get reset GPIO\n",
> + bus->id);
> + return err;
> + }
> + } else {
> + gpiod_set_value_cansleep(gpiod, 1);
> + if (!bus->reset_delay_us)
> + bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
Maybe do this once, where you read the device tree property.
> + udelay(bus->reset_delay_us);
> + gpiod_set_value_cansleep(gpiod, 0);
> + }
> + }
> +
> for (i = 0; i < PHY_MAX_ADDR; i++) {
> if ((bus->phy_mask & (1 << i)) == 0) {
> struct phy_device *phydev;
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 0b29798..83a62e4 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>
> mdio->dev.of_node = np;
>
> + /* Get bus level PHY reset GPIO details */
> + of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
If the property does not exist, it is guaranteed mdio->reset_delay_us
is not changed. So you can set it to the default value before, then do
this call.
Andrew
^ permalink raw reply
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Jamal Hadi Salim @ 2017-04-19 11:32 UTC (permalink / raw)
To: Wolfgang Bumiller, Cong Wang
Cc: Linux Kernel Network Developers, David S. Miller
In-Reply-To: <462700507.23.1492589388406@webmail.proxmox.com>
On 17-04-19 04:09 AM, Wolfgang Bumiller wrote:
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8cc883c063f0..c7e5e437b847 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -555,6 +555,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> struct nlattr *tb[TCA_ACT_MAX + 1];
> struct nlattr *kind;
> int err;
> + bool created;
>
> if (name == NULL) {
> err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL);
> @@ -603,20 +604,19 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> err = a_o->init(net, nla, est, &a, ovr, bind);
> if (err < 0)
> goto err_mod;
> + created = err == ACT_P_CREATED;
>
> if (name == NULL && tb[TCA_ACT_COOKIE]) {
> int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>
> if (cklen > TC_COOKIE_MAX_SIZE) {
> err = -EINVAL;
> - tcf_hash_release(a, bind);
> - goto err_mod;
> + goto err_release;
> }
>
> if (nla_memdup_cookie(a, tb) < 0) {
> err = -ENOMEM;
> - tcf_hash_release(a, bind);
> - goto err_mod;
> + goto err_release;
> }
> }
>
> @@ -624,11 +624,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> * if it exists and is only bound to in a_o->init() then
> * ACT_P_CREATED is not returned (a zero is).
> */
> - if (err != ACT_P_CREATED)
> + if (!created)
> module_put(a_o->owner);
>
> return a;
>
> +err_release:
> + if (created)
> + tcf_hash_release(a, bind);
> err_mod:
> module_put(a_o->owner);
> err_out:
>
This solves one issue, but I am afraid the issue Cong mentioned is a
possibility still.
Lets say user did a replace and tried to also replace the cookie
in that transaction. The init() succeeds but the cookie allocation
fails. To be correct we'll have to undo the replace i.e something
like uninit() which will restore back the old values.
This is very complex and unnecessary.
My suggestion:
If we move the cookie allocation before init we can save it and
only when init succeeds do we attach it to the action, otherwise
we free it on error path.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next 1/2] Add Cong Wang as TC subsystem co-maintainer
From: Jamal Hadi Salim @ 2017-04-19 11:29 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Eric Dumazet,
Jiri Pirko
In-Reply-To: <CAM_iQpXt8pd=t1=S+PaANyRw2Po2Q04k3FJnZXd4UsBsHc_4gA@mail.gmail.com>
On 17-04-19 02:01 AM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 6:23 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> I am happy and honored to review net sched patches,
>
Thank you for all your contributions so far!
cheers,
jamal
^ 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