* Re: [PATCH net-next] bpf: fix arraymap NULL deref and missing overflow and zero size checks
From: David Miller @ 2014-11-19 21:55 UTC (permalink / raw)
To: ast; +Cc: fengguang.wu, hannes, dborkman, netdev, linux-kernel
In-Reply-To: <1416360736-9531-1-git-send-email-ast@plumgrid.com>
From: Alexei Starovoitov <ast@plumgrid.com>
Date: Tue, 18 Nov 2014 17:32:16 -0800
> - fix NULL pointer dereference:
> kernel/bpf/arraymap.c:41 array_map_alloc() error: potential null dereference 'array'. (kzalloc returns null)
> kernel/bpf/arraymap.c:41 array_map_alloc() error: we previously assumed 'array' could be null (see line 40)
>
> - integer overflow check was missing in arraymap
> (hashmap checks for overflow via kmalloc_array())
>
> - arraymap can round_up(value_size, 8) to zero. check was missing.
>
> - hashmap was missing zero size check as well, since roundup_pow_of_two() can
> truncate into zero
>
> - found a typo in the arraymap comment and unnecessary empty line
>
> Fix all of these issues and make both overflow checks explicit U32 in size.
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> This silly NULL deref bug and missing overflow check was an oversight when
> I refactored the code from two allocations (kmalloc for struct bpf_array and
> kcalloc for array of elements) in the first implementation of arraymap
> into one allocation which is this code.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] ixgbe: Correctly disable vlan filter in promiscuous mode
From: Jeff Kirsher @ 2014-11-19 21:56 UTC (permalink / raw)
To: Tantilov, Emil S
Cc: Vlad Yasevich, netdev@vger.kernel.org, Keller, Jacob E,
Skidmore, Donald C
In-Reply-To: <87618083B2453E4A8714035B62D67992501A1011@FMSMSX105.amr.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 3588 bytes --]
On Wed, 2014-11-19 at 13:50 -0800, Tantilov, Emil S wrote:
> >-----Original Message-----
> >From: Vlad Yasevich [mailto:vyasevich@gmail.com]
> >Sent: Wednesday, November 19, 2014 11:41 AM
> >To: Tantilov, Emil S; netdev@vger.kernel.org
> >Cc: Kirsher, Jeffrey T; Keller, Jacob E; Skidmore, Donald C
> >Subject: Re: [PATCH] ixgbe: Correctly disable vlan filter in
> >promiscuous mode
> >
> >On 11/18/2014 03:24 PM, Tantilov, Emil S wrote:
> >>> -----Original Message-----
> >>> From: netdev-owner@vger.kernel.org [mailto:netdev-
> >>> owner@vger.kernel.org] On Behalf Of Vladislav Yasevich
> >>> Sent: Tuesday, November 18, 2014 11:28 AM
> >>> To: netdev@vger.kernel.org
> >>> Cc: Vladislav Yasevich; Kirsher, Jeffrey T; Keller, Jacob E
> >>> Subject: [PATCH] ixgbe: Correctly disable vlan filter in promiscuous mode
> >>>
> >>> IXGBE adapater seems to require that vlan filtering be enabled if VMDQ
> >>> or SRIOV are enabled. When those functions are disabled,
> >>> vlan filtering may be disabled in promiscuous mode.
> >>>
> >>> Prior to commit a9b8943ee129e11045862d6d6e25c5b63c95403c
> >>> ixgbe: remove vlan_filter_disable and enable functions
> >>>
> >>> the logic was correct. However, after the commit the logic
> >>> got reversed and vlan filtered in now turned on when VMDQ/SRIOV
> >>> is disabled.
> >>>
> >>> This patch changes the condition to enable hw vlan filtered
> >>> when VMDQ or SRIOV is enabled.
> >>>
> >>> Fixes: a9b8943ee129e11045862d6d6e25c5b63c95403c (ixgbe:
> >>> remove
> >>> vlan_filter_disable and enable functions)
> >>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >>> CC: Jacob Keller <jacob.e.keller@intel.com>
> >>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >>> ---
> >>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> index d2df4e3..3f81c7a 100644
> >>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> @@ -3936,8 +3936,8 @@ void ixgbe_set_rx_mode(struct
> >>> net_device *netdev)
> >>> * if SR-IOV and VMDQ are disabled - otherwise ensure
> >>> * that hardware VLAN filters remain enabled.
> >>> */
> >>> - if (!(adapter->flags & (IXGBE_FLAG_VMDQ_ENABLED |
> >>> - IXGBE_FLAG_SRIOV_ENABLED)))
> >>> + if (adapter->flags & (IXGBE_FLAG_VMDQ_ENABLED |
> >>> + IXGBE_FLAG_SRIOV_ENABLED))
> >>> vlnctrl |= (IXGBE_VLNCTRL_VFE |
> >>> IXGBE_VLNCTRL_CFIEN);
> >>> } else {
> >>> if (netdev->flags & IFF_ALLMULTI) {
> >>
> >> The current logic is correct and it's like this on purpose
> >> as it should be obvious by the comment preceding this check.
> >
> >Actually the comment right now does not match what the code
> >is doing.
> >
> >The comment states:
> > /* Only disable hardware filter vlans in promiscuous mode
> > * if SR-IOV and VMDQ are disabled - otherwise ensure
> > * that hardware VLAN filters remain enabled.
> > */
> >
> >However, the code currently will _enable_ vlan filtering if VMDQ/SRIOV
> >is _disabled_ in promiscuous mode.
>
> Actually you're right. Sorry - I misread the patch initially.
> This is indeed a bug in the code.
>
> Acked-by: Emil Tantilov <emil.s.tantilov@intel.com>
>
> Jeff should pick it up.
Consider it done, I have added Vlad's patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net] cxgb4i : Don't block unload/cxgb4 unload when remote closes TCP connection
From: David Miller @ 2014-11-19 21:59 UTC (permalink / raw)
To: anish; +Cc: netdev, linux-scsi, hch, jbottomley, kxie, manojmalviya
In-Reply-To: <1416366591-9699-1-git-send-email-anish@chelsio.com>
From: Anish Bhatt <anish@chelsio.com>
Date: Tue, 18 Nov 2014 19:09:51 -0800
> cxgb4i was returning wrong error and not releasing module reference if remote
> end abruptly closed TCP connection. This prevents the cxgb4 network module from
> being unloaded, further affecting other network drivers dependent on cxgb4
>
> Sending to net as this affects all cxgb4 based network drivers.
>
> Signed-off-by: Anish Bhatt <anish@chelsio.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next v2] enic: support skb->xmit_more
From: David Miller @ 2014-11-19 22:02 UTC (permalink / raw)
To: _govind; +Cc: netdev, ssujith, benve, eric.dumazet
In-Reply-To: <1416382172-28775-1-git-send-email-_govind@gmx.com>
From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Wed, 19 Nov 2014 12:59:32 +0530
> Check and update posted_index only when skb->xmit_more is 0 or tx queue is full.
>
> v2:
> use txq_map instead of skb_get_queue_mapping(skb)
>
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] sky2: use new netdev_rss_key_fill() helper
From: David Miller @ 2014-11-19 22:04 UTC (permalink / raw)
To: ipm; +Cc: netdev, mlindner, stephen, edumazet
In-Reply-To: <1416388011-10491-1-git-send-email-ipm@chirality.org.uk>
From: Ian Morris <ipm@chirality.org.uk>
Date: Wed, 19 Nov 2014 09:06:51 +0000
> Switch to a random RSS key rather than a fixed one.
> Using netdev_rss_key_fill helper also ensures that all ports share
> a common key.
>
> See also commit 960fb622f85180f36d3aff82af53e2be3db2f888.
>
> Signed-off-by: Ian Morris <ipm@chirality.org.uk>
Applied, thank you.
^ permalink raw reply
* Re: [PATCHv3 net] openvswitch: Don't validate IPv6 label masks.
From: Pravin Shelar @ 2014-11-19 22:05 UTC (permalink / raw)
To: Joe Stringer; +Cc: netdev, LKML, dev@openvswitch.org
In-Reply-To: <1416434089-47062-1-git-send-email-joestringer@nicira.com>
On Wed, Nov 19, 2014 at 1:54 PM, Joe Stringer <joestringer@nicira.com> wrote:
> When userspace doesn't provide a mask, OVS datapath generates a fully
> unwildcarded mask for the flow by copying the flow and setting all bits
> in all fields. For IPv6 label, this creates a mask that matches on the
> upper 12 bits, causing the following error:
>
> openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)
>
> This patch ignores the label validation check for masks, avoiding this
> error.
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
Thanks for the fix.
Acked-by: Pravin B Shelar <pshelar@nicira.com>
> ---
> v3: Alternative approach.
> Was "openvswitch: Fix mask generation for IPv6 labels."
> v2: OR lower 20 bits (upper 12 bits remain from earlier memdup)
> ---
> net/openvswitch/flow_netlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index fa4ec2e..089b195 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -690,7 +690,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
> return -EINVAL;
> }
>
> - if (ipv6_key->ipv6_label & htonl(0xFFF00000)) {
> + if (!is_mask && ipv6_key->ipv6_label & htonl(0xFFF00000)) {
> OVS_NLERR("IPv6 flow label %x is out of range (max=%x).\n",
> ntohl(ipv6_key->ipv6_label), (1 << 20) - 1);
> return -EINVAL;
> --
> 1.7.10.4
>
^ permalink raw reply
* [PATCH v4] ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Jiri Bohac @ 2014-11-19 22:05 UTC (permalink / raw)
To: David Miller; +Cc: jbohac, arnd, acme, netdev
In-Reply-To: <20141119.154400.1045032776950540216.davem@davemloft.net>
This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).
When a recvmsg syscall blocks waiting for new data, no data can be sent on the
same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.
This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket
Commit b0d0d915 has simply replaced BKL locking with
lock_sock/release_sock. Unlike now, BKL got unlocked while
sleeping, so a blocking recvmsg did not block a concurrent
sendmsg.
Only keep the socket locked while actually working with the socket data and
release it prior to calling skb_recv_datagram().
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index a0c7536..d0725d9 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1764,6 +1764,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
struct ipxhdr *ipx = NULL;
struct sk_buff *skb;
int copied, rc;
+ bool locked = true;
lock_sock(sk);
/* put the autobinding in */
@@ -1790,6 +1791,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
if (sock_flag(sk, SOCK_ZAPPED))
goto out;
+ release_sock(sk);
+ locked = false;
skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
flags & MSG_DONTWAIT, &rc);
if (!skb) {
@@ -1825,7 +1828,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
out_free:
skb_free_datagram(sk, skb);
out:
- release_sock(sk);
+ if (locked)
+ release_sock(sk);
return rc;
}
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
* Re: [PATCH v4] ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Arnd Bergmann @ 2014-11-19 22:12 UTC (permalink / raw)
To: Jiri Bohac; +Cc: David Miller, acme, netdev
In-Reply-To: <20141119220549.GA26133@midget.suse.cz>
On Wednesday 19 November 2014 23:05:49 Jiri Bohac wrote:
> This fixes an old regression introduced by commit
> b0d0d915 (ipx: remove the BKL).
>
> When a recvmsg syscall blocks waiting for new data, no data can be sent on the
> same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.
>
> This breaks mars-nwe (NetWare emulator):
> - the ncpserv process reads the request using recvmsg
> - ncpserv forks and spawns nwconn
> - ncpserv calls a (blocking) recvmsg and waits for new requests
> - nwconn deadlocks in sendmsg on the same socket
>
> Commit b0d0d915 has simply replaced BKL locking with
> lock_sock/release_sock. Unlike now, BKL got unlocked while
> sleeping, so a blocking recvmsg did not block a concurrent
> sendmsg.
>
> Only keep the socket locked while actually working with the socket data and
> release it prior to calling skb_recv_datagram().
>
>
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Cong Wang @ 2014-11-19 22:26 UTC (permalink / raw)
To: Wengang; +Cc: Eric Dumazet, netdev
In-Reply-To: <546C4022.5010509@oracle.com>
On Tue, Nov 18, 2014 at 11:00 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>
> Yes, that's true. So the simplest way is move ipoib_header_ops to vmlinux.
>
That is not an option. Perhaps you need RCU to protect the dev->header_ops
pointer.
^ permalink raw reply
* [PATCH RESEND] natsemi: fix timing issue for reading mac from eeprom via dp8381x
From: Roland Kletzing @ 2014-11-19 22:26 UTC (permalink / raw)
To: netdev; +Cc: David Miller
Natsemi driver does not read MAC correctly from eeprom, while natsemi-diag
from nictools-pci does. Apparently, it`s a timing issue in the kernel driver.
According to ftp://ftp.gwdg.de/pub/linux/misc/donald.becker/diag/natsemi-diag.c
, eeprom_delay(ee_addr) is defined as follows:
/* Delay between EEPROM clock transitions.
This flushes the write buffer to prevent quick double-writes.
*/
#define eeprom_delay(ee_addr) inl(ee_addr); inl(ee_addr)
while in the natsemi linux kernel driver, the delay is done this way :
#define eeprom_delay(ee_addr) readl(ee_addr)
, which results in the MAC being all zero`s on my systems (old Geode GX1 board
like being used in Compaq Evo T20 or Wyse WT3235LE Thin Clients).
So i simply added a second readl() to increase delay (instead of turning into
inl() as proposed before). This may look a little bit ugly, but it`s fixing the
problem for me.
I´m not sure how many natsemi users being left on this planet (probably few),
but i guess this change does not do any harm on platforms where the driver does
not behave buggy, so please consider adding it to mainline/stable/longterm.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=51791
Reported-by: Roland Kletzing <devzero@web.de>
Signed-off-by: Roland Kletzing <devzero@web.de>
---
drivers/net/ethernet/natsemi/natsemi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/natsemi/natsemi.c b/drivers/net/ethernet/natsemi/natsemi.c
index b83f7c0..96f0029 100644
--- a/drivers/net/ethernet/natsemi/natsemi.c
+++ b/drivers/net/ethernet/natsemi/natsemi.c
@@ -987,7 +987,7 @@ static int natsemi_probe1(struct pci_dev *pdev, const struct pci_device_id *ent)
The old method of using an ISA access as a delay, __SLOW_DOWN_IO__, is
deprecated.
*/
-#define eeprom_delay(ee_addr) readl(ee_addr)
+#define eeprom_delay(ee_addr) readl(ee_addr); readl(ee_addr)
#define EE_Write0 (EE_ChipSelect)
#define EE_Write1 (EE_ChipSelect | EE_DataIn)
--
^ permalink raw reply related
* Re: [PATCH 1/1] netfilter: Deletion of unnecessary checks before two function calls
From: Julian Anastasov @ 2014-11-19 22:26 UTC (permalink / raw)
To: SF Markus Elfring
Cc: David S. Miller, Jozsef Kadlecsik, Pablo Neira Ayuso,
Patrick McHardy, Simon Horman, Wensong Zhang, netdev, lvs-devel,
netfilter-devel, coreteam, LKML, kernel-janitors, Coccinelle
In-Reply-To: <546BA253.6030100@users.sourceforge.net>
Hello,
On Tue, 18 Nov 2014, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 18 Nov 2014 20:37:05 +0100
>
> The functions free_percpu() and module_put() test whether their argument
> is NULL and then return immediately. Thus the test around the call is
> not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Pablo, the IPVS parts look ok to me,
Acked-by: Julian Anastasov <ja@ssi.bg>
> ---
> net/netfilter/ipvs/ip_vs_ctl.c | 3 +--
> net/netfilter/ipvs/ip_vs_pe.c | 3 +--
> net/netfilter/ipvs/ip_vs_sched.c | 3 +--
> net/netfilter/ipvs/ip_vs_sync.c | 3 +--
> net/netfilter/nf_tables_api.c | 3 +--
> 5 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index fd3f444..7c5e40a 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@ -465,8 +465,7 @@ __ip_vs_bind_svc(struct ip_vs_dest *dest, struct ip_vs_service *svc)
>
> static void ip_vs_service_free(struct ip_vs_service *svc)
> {
> - if (svc->stats.cpustats)
> - free_percpu(svc->stats.cpustats);
> + free_percpu(svc->stats.cpustats);
> kfree(svc);
> }
>
> diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c
> index 1a82b29..0df17ca 100644
> --- a/net/netfilter/ipvs/ip_vs_pe.c
> +++ b/net/netfilter/ipvs/ip_vs_pe.c
> @@ -37,8 +37,7 @@ struct ip_vs_pe *__ip_vs_pe_getbyname(const char *pe_name)
> rcu_read_unlock();
> return pe;
> }
> - if (pe->module)
> - module_put(pe->module);
> + module_put(pe->module);
> }
> rcu_read_unlock();
>
> diff --git a/net/netfilter/ipvs/ip_vs_sched.c b/net/netfilter/ipvs/ip_vs_sched.c
> index 4dbcda6..199760c 100644
> --- a/net/netfilter/ipvs/ip_vs_sched.c
> +++ b/net/netfilter/ipvs/ip_vs_sched.c
> @@ -104,8 +104,7 @@ static struct ip_vs_scheduler *ip_vs_sched_getbyname(const char *sched_name)
> mutex_unlock(&ip_vs_sched_mutex);
> return sched;
> }
> - if (sched->module)
> - module_put(sched->module);
> + module_put(sched->module);
> }
>
> mutex_unlock(&ip_vs_sched_mutex);
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index eadffb2..cafe28d 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -820,8 +820,7 @@ ip_vs_conn_fill_param_sync(struct net *net, int af, union ip_vs_sync_conn *sc,
>
> p->pe_data = kmemdup(pe_data, pe_data_len, GFP_ATOMIC);
> if (!p->pe_data) {
> - if (p->pe->module)
> - module_put(p->pe->module);
> + module_put(p->pe->module);
> return -ENOMEM;
> }
> p->pe_data_len = pe_data_len;
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index deeb95f..b115f54 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -3472,8 +3472,7 @@ static int nf_tables_abort(struct sk_buff *skb)
> break;
> case NFT_MSG_NEWCHAIN:
> if (nft_trans_chain_update(trans)) {
> - if (nft_trans_chain_stats(trans))
> - free_percpu(nft_trans_chain_stats(trans));
> + free_percpu(nft_trans_chain_stats(trans));
>
> nft_trans_destroy(trans);
> } else {
> --
> 2.1.3
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Cong Wang @ 2014-11-19 22:56 UTC (permalink / raw)
To: Wengang; +Cc: Eric Dumazet, netdev
In-Reply-To: <CAHA+R7OyA_V149bpE8qUgG9epU-5UG8cf=zOgh5ZXVB3ZFZHUw@mail.gmail.com>
On Wed, Nov 19, 2014 at 2:26 PM, Cong Wang <cwang@twopensource.com> wrote:
> On Tue, Nov 18, 2014 at 11:00 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>>
>> Yes, that's true. So the simplest way is move ipoib_header_ops to vmlinux.
>>
>
> That is not an option. Perhaps you need RCU to protect the dev->header_ops
> pointer.
Or maybe take a refcount of that module.
BTW, why this is a problem only for bonding? Doesn't neigh have the
same bug? Or it takes the module refcount somewhere I don't find?
^ permalink raw reply
* RE: [net-next 07/15] ixgbevf: Change receive model to use double buffered page based receives
From: Tantilov, Emil S @ 2014-11-19 23:06 UTC (permalink / raw)
To: Alexander Duyck, Kirsher, Jeffrey T, davem@davemloft.net
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com
In-Reply-To: <546CE067.9010903@redhat.com>
>-----Original Message-----
>From: Alexander Duyck [mailto:alexander.h.duyck@redhat.com]
>Sent: Wednesday, November 19, 2014 10:25 AM
>To: Kirsher, Jeffrey T; davem@davemloft.net
>Cc: Tantilov, Emil S; netdev@vger.kernel.org;
>nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com
>Subject: Re: [net-next 07/15] ixgbevf: Change receive model to use double buffered page based receives
>
>There were a few things in this patch that should be addressed.
>
>Comments inline below.
>
>- Alex
>
>On 11/18/2014 08:10 PM, Jeff Kirsher wrote:
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> This patch changes the basic receive path for ixgbevf so that instead of
>> receiving the data into an skb it is received into a double buffered page.
>> The main change is that the receives will be done in pages only and then
>> pull the header out of the page and copy it into the sk_buff data.
>>
>> This has the advantages of reduced cache misses and improved performance on
>> IOMMU enabled systems.
>>
>> CC: Alexander Duyck <alexander.h.duyck@redhat.com>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 24 +-
>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 452 +++++++++++++++-------
>> 2 files changed, 331 insertions(+), 145 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> index 72a354b..2362001 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> @@ -58,8 +58,9 @@ struct ixgbevf_tx_buffer {
>> };
>>
>> struct ixgbevf_rx_buffer {
>> - struct sk_buff *skb;
>> dma_addr_t dma;
>> + struct page *page;
>> + unsigned int page_offset;
>> };
>>
>> struct ixgbevf_stats {
>> @@ -91,9 +92,10 @@ struct ixgbevf_ring {
>> void *desc; /* descriptor ring memory */
>> dma_addr_t dma; /* phys. address of
>descriptor ring */
>> unsigned int size; /* length in bytes */
>> - unsigned int count; /* amount of descriptors */
>> - unsigned int next_to_use;
>> - unsigned int next_to_clean;
>> + u16 count; /* amount of descriptors */
>> + u16 next_to_use;
>> + u16 next_to_clean;
>> + u16 next_to_alloc;
>>
>> union {
>> struct ixgbevf_tx_buffer *tx_buffer_info;
>> @@ -109,12 +111,11 @@ struct ixgbevf_ring {
>>
>> u64 hw_csum_rx_error;
>> u8 __iomem *tail;
>> + struct sk_buff *skb;
>>
>> u16 reg_idx; /* holds the special value that gets the hardware register
>> * offset associated with this ring, which is different
>> * for DCB and RSS modes */
>> -
>> - u16 rx_buf_len;
>> int queue_index; /* needed for multiqueue queue management */
>> };
>>
>> @@ -133,12 +134,10 @@ struct ixgbevf_ring {
>>
>> /* Supported Rx Buffer Sizes */
>> #define IXGBEVF_RXBUFFER_256 256 /* Used for packet split */
>> -#define IXGBEVF_RXBUFFER_2K 2048
>> -#define IXGBEVF_RXBUFFER_4K 4096
>> -#define IXGBEVF_RXBUFFER_8K 8192
>> -#define IXGBEVF_RXBUFFER_10K 10240
>> +#define IXGBEVF_RXBUFFER_2048 2048
>>
>> #define IXGBEVF_RX_HDR_SIZE IXGBEVF_RXBUFFER_256
>> +#define IXGBEVF_RX_BUFSZ IXGBEVF_RXBUFFER_2048
>>
>> #define MAXIMUM_ETHERNET_VLAN_SIZE (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
>>
>> @@ -430,11 +429,6 @@ enum ixbgevf_state_t {
>> __IXGBEVF_WORK_INIT,
>> };
>>
>> -struct ixgbevf_cb {
>> - struct sk_buff *prev;
>> -};
>> -#define IXGBE_CB(skb) ((struct ixgbevf_cb *)(skb)->cb)
>> -
>> enum ixgbevf_boards {
>> board_82599_vf,
>> board_X540_vf,
>> diff --git
>a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> index 20bebd2..2ca7c96 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> @@ -422,8 +422,7 @@ static void
>ixgbevf_process_skb_fields(struct ixgbevf_ring *rx_ring,
>> * that this is in fact a non-EOP buffer.
>> **/
>> static bool ixgbevf_is_non_eop(struct ixgbevf_ring *rx_ring,
>> - union ixgbe_adv_rx_desc *rx_desc,
>> - struct sk_buff *skb)
>> + union ixgbe_adv_rx_desc *rx_desc)
>> {
>> u32 ntc = rx_ring->next_to_clean + 1;
>>
>> @@ -439,37 +438,40 @@ static bool
>ixgbevf_is_non_eop(struct ixgbevf_ring *rx_ring,
>> return true;
>> }
>>
>> -static bool ixgbevf_alloc_mapped_skb(struct ixgbevf_ring *rx_ring,
>> - struct ixgbevf_rx_buffer *bi)
>> +static bool ixgbevf_alloc_mapped_page(struct ixgbevf_ring *rx_ring,
>> + struct ixgbevf_rx_buffer *bi)
>> {
>> - struct sk_buff *skb = bi->skb;
>> + struct page *page = bi->page;
>> dma_addr_t dma = bi->dma;
>>
>> - if (unlikely(skb))
>> + /* since we are recycling buffers we should seldom need to alloc */
>> + if (likely(page))
>> return true;
>>
>> - skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
>> - rx_ring->rx_buf_len);
>> - if (unlikely(!skb)) {
>> - rx_ring->rx_stats.alloc_rx_buff_failed++;
>> + /* alloc new page for storage */
>> + page = dev_alloc_page();
>> + if (unlikely(!page)) {
>> + rx_ring->rx_stats.alloc_rx_page_failed++;
>> return false;
>> }
>>
>> - dma = dma_map_single(rx_ring->dev, skb->data,
>> - rx_ring->rx_buf_len, DMA_FROM_DEVICE);
>> + /* map page for use */
>> + dma = dma_map_page(rx_ring->dev, page, 0,
>> + PAGE_SIZE, DMA_FROM_DEVICE);
>>
>> /* if mapping failed free memory back to system since
>> * there isn't much point in holding memory we can't use
>> */
>> if (dma_mapping_error(rx_ring->dev, dma)) {
>> - dev_kfree_skb_any(skb);
>> + __free_page(page);
>>
>> rx_ring->rx_stats.alloc_rx_buff_failed++;
>> return false;
>> }
>>
>> - bi->skb = skb;
>> bi->dma = dma;
>> + bi->page = page;
>> + bi->page_offset = 0;
>>
>> return true;
>> }
>> @@ -495,13 +497,13 @@ static void
>ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
>> i -= rx_ring->count;
>>
>> do {
>> - if (!ixgbevf_alloc_mapped_skb(rx_ring, bi))
>> + if (!ixgbevf_alloc_mapped_page(rx_ring, bi))
>> break;
>>
>> /* Refresh the desc even if pkt_addr didn't change
>> * because each write-back erases this info.
>> */
>> - rx_desc->read.pkt_addr = cpu_to_le64(bi->dma);
>> + rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
>>
>> rx_desc++;
>> bi++;
>> @@ -524,6 +526,9 @@ static void
>ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
>> /* record the next descriptor to use */
>> rx_ring->next_to_use = i;
>>
>> + /* update next to alloc since we have filled the ring */
>> + rx_ring->next_to_alloc = i;
>> +
>> /* Force memory writes to complete before letting h/w
>> * know there are new descriptors to fetch. (Only
>> * applicable for weak-ordered memory model archs,
>> @@ -534,6 +539,257 @@ static void
>ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
>> }
>> }
>>
>> +/* ixgbevf_pull_tail - ixgbevf specific version of skb_pull_tail
>> + * @rx_ring: Rx descriptor ring packet is being transacted on
>> + * @skb: pointer to current skb being adjusted
>> + *
>> + * This function is an ixgbevf specific version of __pskb_pull_tail. The
>> + * main difference between this version and the original function is that
>> + * this function can make several assumptions about the state of things
>> + * that allow for significant optimizations versus the standard function.
>> + * As a result we can do things like drop a frag and maintain an accurate
>> + * truesize for the skb.
>> + */
>> +static void ixgbevf_pull_tail(struct ixgbevf_ring *rx_ring,
>> + struct sk_buff *skb)
>> +{
>> + struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[0];
>> + unsigned char *va;
>> + unsigned int pull_len;
>> +
>> + /* it is valid to use page_address instead of kmap since we are
>> + * working with pages allocated out of the lomem pool per
>> + * alloc_page(GFP_ATOMIC)
>> + */
>> + va = skb_frag_address(frag);
>> +
>> + /* we need the header to contain the greater of either ETH_HLEN or
>> + * 60 bytes if the skb->len is less than 60 for skb_pad.
>> + */
>> + pull_len = eth_get_headlen(va, IXGBEVF_RX_HDR_SIZE);
>> +
>> + /* align pull length to size of long to optimize memcpy performance */
>> + skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
>> +
>> + /* update all of the pointers */
>> + skb_frag_size_sub(frag, pull_len);
>> + frag->page_offset += pull_len;
>> + skb->data_len -= pull_len;
>> + skb->tail += pull_len;
>> +}
>
>I really think we should look at making this into a generic function.
>Maybe I will submit something later today to get a common function
>placed in the code. Maybe something like eth_pull_tail.
Sounds good.
>
>> +/* ixgbevf_cleanup_headers - Correct corrupted or empty headers
>> + * @rx_ring: Rx descriptor ring packet is being transacted on
>> + * @rx_desc: pointer to the EOP Rx descriptor
>> + * @skb: pointer to current skb being fixed
>> + *
>> + * Check for corrupted packet headers caused by senders on the local L2
>> + * embedded NIC switch not setting up their Tx Descriptors right. These
>> + * should be very rare.
>> + *
>> + * Also address the case where we are pulling data in on pages only
>> + * and as such no data is present in the skb header.
>> + *
>> + * In addition if skb is not at least 60 bytes we need to pad it so that
>> + * it is large enough to qualify as a valid Ethernet frame.
>> + *
>> + * Returns true if an error was encountered and skb was freed.
>> + */
>> +static bool ixgbevf_cleanup_headers(struct ixgbevf_ring *rx_ring,
>> + union ixgbe_adv_rx_desc *rx_desc,
>> + struct sk_buff *skb)
>> +{
>> + /* verify that the packet does not have any known errors */
>> + if (unlikely(ixgbevf_test_staterr(rx_desc,
>> +
>IXGBE_RXDADV_ERR_FRAME_ERR_MASK))) {
>> + struct net_device *netdev = rx_ring->netdev;
>> +
>> + if (!(netdev->features & NETIF_F_RXALL)) {
>> + dev_kfree_skb_any(skb);
>> + return true;
>> + }
>> + }
>> +
>> + /* place header in linear portion of buffer */
>> + if (skb_is_nonlinear(skb))
>> + ixgbevf_pull_tail(rx_ring, skb);
>> +
>> + /* if skb_pad returns an error the skb was freed */
>> + if (unlikely(skb->len < 60)) {
>> + int pad_len = 60 - skb->len;
>> +
>> + if (skb_pad(skb, pad_len))
>> + return true;
>> + __skb_put(skb, pad_len);
>> + }
>> +
>> + return false;
>> +}
>
>The same goes for the padding bit here. Maybe something like eth_skb_pad.
OK
>> +/* ixgbevf_reuse_rx_page - page flip buffer and store it back on the ring
>> + * @rx_ring: Rx descriptor ring to store buffers on
>> + * @old_buff: donor buffer to have page reused
>> + *
>> + * Synchronizes page for reuse by the adapter
>> + */
>> +static void ixgbevf_reuse_rx_page(struct ixgbevf_ring *rx_ring,
>> + struct ixgbevf_rx_buffer *old_buff)
>> +{
>> + struct ixgbevf_rx_buffer *new_buff;
>> + u16 nta = rx_ring->next_to_alloc;
>> +
>> + new_buff = &rx_ring->rx_buffer_info[nta];
>> +
>> + /* update, and store next to alloc */
>> + nta++;
>> + rx_ring->next_to_alloc = (nta < rx_ring->count) ? nta : 0;
>> +
>> + /* transfer page from old buffer to new buffer */
>> + new_buff->page = old_buff->page;
>> + new_buff->dma = old_buff->dma;
>> + new_buff->page_offset = old_buff->page_offset;
>> +
>> + /* sync the buffer for use by the device */
>> + dma_sync_single_range_for_device(rx_ring->dev, new_buff->dma,
>> + new_buff->page_offset,
>> + IXGBEVF_RX_BUFSZ,
>> + DMA_FROM_DEVICE);
>> +}
>> +
>> +/* ixgbevf_add_rx_frag - Add contents of Rx buffer to sk_buff
>> + * @rx_ring: Rx descriptor ring to transact packets on
>> + * @rx_buffer: buffer containing page to add
>> + * @rx_desc: descriptor containing length of buffer written by hardware
>> + * @skb: sk_buff to place the data into
>> + *
>> + * This function will add the data contained in rx_buffer->page to the skb.
>> + * This is done either through a direct copy if the data in the buffer is
>> + * less than the skb header size, otherwise it will just attach the page as
>> + * a frag to the skb.
>> + *
>> + * The function will then update the page offset if necessary and return
>> + * true if the buffer can be reused by the adapter.
>> + */
>> +static bool ixgbevf_add_rx_frag(struct ixgbevf_ring *rx_ring,
>> + struct ixgbevf_rx_buffer *rx_buffer,
>> + union ixgbe_adv_rx_desc *rx_desc,
>> + struct sk_buff *skb)
>> +{
>> + struct page *page = rx_buffer->page;
>> + unsigned int size = le16_to_cpu(rx_desc-
>>wb.upper.length);
>> +#if (PAGE_SIZE < 8192)
>> + unsigned int truesize = IXGBEVF_RX_BUFSZ;
>> +#else
>> + unsigned int truesize = ALIGN(size, L1_CACHE_BYTES);
>> +#endif
>> +
>> + if ((size <= IXGBEVF_RX_HDR_SIZE) && !skb_is_nonlinear(skb)) {
>> + unsigned char *va = page_address(page) + rx_buffer->page_offset;
>> +
>> + memcpy(__skb_put(skb, size), va, ALIGN(size, sizeof(long)));
>> +
>> + /* we can reuse buffer as-is, just make sure it is local */
>> + if (likely(page_to_nid(page) == numa_node_id()))
>> + return true;
>> +
>> + /* this page cannot be reused so discard it */
>> + put_page(page);
>> + return false;
>> + }
>> +
>> + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>> + rx_buffer->page_offset, size, truesize);
>> +
>> + /* avoid re-using remote pages */
>> + if (unlikely(page_to_nid(page) != numa_node_id()))
>> + return false;
>> +
>
>This is missing the pfmemalloc fix that is already in igb,
>and that I submitted for ixgbe and fm10k.
Just because of timing. I can redo the patch with the pfmemalloc check added.
>
>> +#if (PAGE_SIZE < 8192)
>> + /* if we are only owner of page we can reuse it */
>> + if (unlikely(page_count(page) != 1))
>> + return false;
>> +
>> + /* flip page offset to other buffer */
>> + rx_buffer->page_offset ^= IXGBEVF_RX_BUFSZ;
>> +
>> + /* since we are the only owner of the page and we need to
>> + * increment it.
>> + */
>> + atomic_inc(&page->_count);
>> +#else
>> + /* move offset up to the next cache line */
>> + rx_buffer->page_offset += truesize;
>> +
>> + if (rx_buffer->page_offset > (PAGE_SIZE - IXGBEVF_RX_BUFSZ))
>> + return false;
>> +
>> + /* bump ref count on page before it is given to the stack */
>> + get_page(page);
>> +#endif
>> +
>> + return true;
>> +}
>
>The get_page and atomic_inc calls can be pulled out and
>placed after if #if/else logic. The preference is to use atomic_inc since
>you are using an order 0 page and don't need to check for PageTail().
I can do that,
>> +static struct sk_buff *ixgbevf_fetch_rx_buffer(struct ixgbevf_ring *rx_ring,
>> + union ixgbe_adv_rx_desc *rx_desc,
>> + struct sk_buff *skb)
>> +{
>> + struct ixgbevf_rx_buffer *rx_buffer;
>> + struct page *page;
>> +
>> + rx_buffer = &rx_ring->rx_buffer_info[rx_ring->next_to_clean];
>> + page = rx_buffer->page;
>> + prefetchw(page);
>> +
>> + if (likely(!skb)) {
>> + void *page_addr = page_address(page) +
>> + rx_buffer->page_offset;
>> +
>> + /* prefetch first cache line of first page */
>> + prefetch(page_addr);
>> +#if L1_CACHE_BYTES < 128
>> + prefetch(page_addr + L1_CACHE_BYTES);
>> +#endif
>> +
>> + /* allocate a skb to store the frags */
>> + skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
>> + IXGBEVF_RX_HDR_SIZE);
>> + if (unlikely(!skb)) {
>> + rx_ring->rx_stats.alloc_rx_buff_failed++;
>> + return NULL;
>> + }
>> +
>> + /* we will be copying header into skb->data in
>> + * pskb_may_pull so it is in our interest to prefetch
>> + * it now to avoid a possible cache miss
>> + */
>> + prefetchw(skb->data);
>> + }
>> +
>> + /* we are reusing so sync this buffer for CPU use */
>> + dma_sync_single_range_for_cpu(rx_ring->dev,
>> + rx_buffer->dma,
>> + rx_buffer->page_offset,
>> + IXGBEVF_RX_BUFSZ,
>> + DMA_FROM_DEVICE);
>> +
>> + /* pull page into skb */
>> + if (ixgbevf_add_rx_frag(rx_ring, rx_buffer, rx_desc, skb)) {
>> + /* hand second half of page back to the ring */
>> + ixgbevf_reuse_rx_page(rx_ring, rx_buffer);
>> + } else {
>> + /* we are not reusing the buffer so unmap it */
>> + dma_unmap_page(rx_ring->dev, rx_buffer->dma,
>> + PAGE_SIZE, DMA_FROM_DEVICE);
>> + }
>> +
>> + /* clear contents of buffer_info */
>> + rx_buffer->dma = 0;
>> + rx_buffer->page = NULL;
>> +
>> + return skb;
>> +}
>> +
>> static inline void ixgbevf_irq_enable_queues(struct ixgbevf_adapter *adapter,
>> u32 qmask)
>> {
>
><snip>
>
>> @@ -3320,21 +3522,11 @@ static int ixgbevf_set_mac(struct net_device *netdev, void *p)
>> static int ixgbevf_change_mtu(struct net_device *netdev, int new_mtu)
>> {
>> struct ixgbevf_adapter *adapter = netdev_priv(netdev);
>> + struct ixgbe_hw *hw = &adapter->hw;
>> int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
>> - int max_possible_frame = MAXIMUM_ETHERNET_VLAN_SIZE;
>> -
>> - switch (adapter->hw.api_version) {
>> - case ixgbe_mbox_api_11:
>> - max_possible_frame = IXGBE_MAX_JUMBO_FRAME_SIZE;
>> - break;
>> - default:
>> - if (adapter->hw.mac.type == ixgbe_mac_X540_vf)
>> - max_possible_frame = IXGBE_MAX_JUMBO_FRAME_SIZE;
>> - break;
>> - }
>>
>> /* MTU < 68 is an error and causes problems on some kernels */
>> - if ((new_mtu < 68) || (max_frame > max_possible_frame))
>> + if ((new_mtu < 68) || (max_frame > IXGBE_MAX_JUMBO_FRAME_SIZE))
>> return -EINVAL;
>>
>> hw_dbg(&adapter->hw, "changing MTU from %d to %d\n",
>
>This is wrong. You are still limited by the PF so if it is a version
>1.0 mailbox on an 82599 you cannot enable jumbo frames. Yes you can
>support it but the PF won't let you do it.
>
>> @@ -3342,8 +3534,8 @@ static int ixgbevf_change_mtu(struct
>net_device *netdev, int new_mtu)
>> /* must set new MTU before calling down or up */
>> netdev->mtu = new_mtu;
>>
>> - if (netif_running(netdev))
>> - ixgbevf_reinit_locked(adapter);
>> + /* notify the PF of our intent to use this size of frame */
>> + ixgbevf_rlpml_set_vf(hw, max_frame);
>>
>> return 0;
>> }
>>
>
>This is the reason why the change is wrong. If the mailbox api is
>version 1.0 you cannot support jumbo frames so ixgbevf_rlmpl_set_vf will
>return an error via the mailbox indicating that the message >is not
>supported.
I'll have to see how this change was introduced, but I can drop it for now since it's not directly related to this change anyway.
Thanks,
Emil
^ permalink raw reply
* Re: [PATCH net-next] bridge: make proxy arp configurable
From: kyeyoonp @ 2014-11-19 23:11 UTC (permalink / raw)
To: David Miller; +Cc: shemming, kyeyoonp, netdev
In-Reply-To: <20141031.122133.1697556792716394439.davem@davemloft.net>
> From: Stephen Hemminger <shemming@brocade.com>
> Date: Thu, 30 Oct 2014 20:09:42 -0700
>
>> @@ -60,3 +60,19 @@ config BRIDGE_VLAN_FILTERING
>> Say N to exclude this support and reduce the binary size.
>>
>> If unsure, say Y.
>> +
>> +config BRIDGE_ARP_PROXY
>> + bool "ARP proxying"
>> + depends on BRIDGE
>> + depends on INET
>> + default y
>> + ---help---
>> + If you say Y here, then the Ethernet bridge to keep track of
>> + the hardware address to IP address mapping.
>> +
>> + It is most useful when used as a wireless AP.
>> +
>> + Say N to exclude this support and reduce the binary size.
>> +
>> + If unsure, say Y.
>> +
>
> Please do not ever add empty lines at the end of files, GIT warns
> about this when I try to apply your patch.
Hi Dave, I notice that this patch was never applied to "net-next". I
have one more patch contribution to make to the bridge code regarding
Proxy ARP IPv4. I would very much like this patch by Stephen to be
applied as well. I have no problem resubmitting this patch (with the
empty line removed). Do you want me to do that as part of the set?
Thanks!
- Kyeyoon
^ permalink raw reply
* [PATCH net-next] net: sctp: keep owned chunk in destructor_arg instead of skb->cb
From: Daniel Borkmann @ 2014-11-19 23:21 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev
It's just silly to hold the skb destructor argument around inside
skb->cb[] as we currently do in SCTP.
Though this has been around forever, I'm inclined to say that prior
to 4c3a5bdae293 ("sctp: Don't charge for data in sndbuf again when
transmitting packet") this may well have caused issues as doing so
violates the cb[] usage accross layers; before 4c3a5bdae293-times,
we have charged twice for data, and when destructor kicks in, cb[]
could have been overwritten already by someone else.
Nowadays, we're sort of cheating on data accounting in the sense
that due to commit 4c3a5bdae293, we orphan the skb already in the
SCTP output path, and use a different destructor only to make sure
the sk doesn't vanish on skb destruction time. Thus, cb[] is still
valid here as we operate within the SCTP layer. It's actually a big
candidate for future rework, imho.
Anyhow, lets keep the chunk in destructor_arg, as this is the actual
purpose for it so that in future, we don't run into trouble.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
net/sctp/socket.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 2120292..85e0b65 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -162,7 +162,7 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk)
chunk->skb->destructor = sctp_wfree;
/* Save the chunk pointer in skb for sctp_wfree to use later. */
- *((struct sctp_chunk **)(chunk->skb->cb)) = chunk;
+ skb_shinfo(chunk->skb)->destructor_arg = chunk;
asoc->sndbuf_used += SCTP_DATA_SNDSIZE(chunk) +
sizeof(struct sk_buff) +
@@ -6870,14 +6870,10 @@ static void sctp_wake_up_waiters(struct sock *sk,
*/
static void sctp_wfree(struct sk_buff *skb)
{
- struct sctp_association *asoc;
- struct sctp_chunk *chunk;
- struct sock *sk;
+ struct sctp_chunk *chunk = skb_shinfo(skb)->destructor_arg;
+ struct sctp_association *asoc = chunk->asoc;
+ struct sock *sk = asoc->base.sk;
- /* Get the saved chunk pointer. */
- chunk = *((struct sctp_chunk **)(skb->cb));
- asoc = chunk->asoc;
- sk = asoc->base.sk;
asoc->sndbuf_used -= SCTP_DATA_SNDSIZE(chunk) +
sizeof(struct sk_buff) +
sizeof(struct sctp_chunk);
--
1.7.11.7
^ permalink raw reply related
* [net-next PATCH v2] ixgbe: Remove IXGBE_FLAG_IN_NETPOLL since it doesn't do anything
From: Alexander Duyck @ 2014-11-19 23:28 UTC (permalink / raw)
To: netdev; +Cc: donald.c.skidmore, jeffrey.t.kirsher
This patch removes some dead code from the cleanup path for ixgbe.
Setting and clearing the flag doesn't do anything since all we are doing is
setting the flag, scheduling napi, clearing the flag and then letting
netpoll do the polling cleanup. As such it doesn't make much sense to have
it there.
This patch also removes one minor white-space error.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
v2: Fixed an unused variable warning for adapter in ixgbe_rx_skb
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 18 ++++--------------
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 86fa607..f7d46b3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -610,7 +610,6 @@ struct ixgbe_adapter {
#define IXGBE_FLAG_RX_1BUF_CAPABLE (u32)(1 << 4)
#define IXGBE_FLAG_RX_PS_CAPABLE (u32)(1 << 5)
#define IXGBE_FLAG_RX_PS_ENABLED (u32)(1 << 6)
-#define IXGBE_FLAG_IN_NETPOLL (u32)(1 << 7)
#define IXGBE_FLAG_DCA_ENABLED (u32)(1 << 8)
#define IXGBE_FLAG_DCA_CAPABLE (u32)(1 << 9)
#define IXGBE_FLAG_IMIR_ENABLED (u32)(1 << 10)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7c4d3b3..3dfec61 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1583,14 +1583,10 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
static void ixgbe_rx_skb(struct ixgbe_q_vector *q_vector,
struct sk_buff *skb)
{
- struct ixgbe_adapter *adapter = q_vector->adapter;
-
if (ixgbe_qv_busy_polling(q_vector))
netif_receive_skb(skb);
- else if (!(adapter->flags & IXGBE_FLAG_IN_NETPOLL))
- napi_gro_receive(&q_vector->napi, skb);
else
- netif_rx(skb);
+ napi_gro_receive(&q_vector->napi, skb);
}
/**
@@ -6064,7 +6060,6 @@ static void ixgbe_check_hang_subtask(struct ixgbe_adapter *adapter)
/* Cause software interrupt to ensure rings are cleaned */
ixgbe_irq_rearm_queues(adapter, eics);
-
}
/**
@@ -7397,14 +7392,9 @@ static void ixgbe_netpoll(struct net_device *netdev)
if (test_bit(__IXGBE_DOWN, &adapter->state))
return;
- adapter->flags |= IXGBE_FLAG_IN_NETPOLL;
- if (adapter->flags & IXGBE_FLAG_MSIX_ENABLED) {
- for (i = 0; i < adapter->num_q_vectors; i++)
- ixgbe_msix_clean_rings(0, adapter->q_vector[i]);
- } else {
- ixgbe_intr(adapter->pdev->irq, netdev);
- }
- adapter->flags &= ~IXGBE_FLAG_IN_NETPOLL;
+ /* loop through and schedule all active queues */
+ for (i = 0; i < adapter->num_q_vectors; i++)
+ ixgbe_msix_clean_rings(0, adapter->q_vector[i]);
}
#endif
^ permalink raw reply related
* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Pravin Shelar @ 2014-11-19 23:34 UTC (permalink / raw)
To: Joe Stringer; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <1415906275-3172-13-git-send-email-joestringer@nicira.com>
On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com> wrote:
> Previously, flows were manipulated by userspace specifying a full,
> unmasked flow key. This adds significant burden onto flow
> serialization/deserialization, particularly when dumping flows.
>
> This patch adds an alternative way to refer to flows using a
> variable-length "unique flow identifier" (UFID). At flow setup time,
> userspace may specify a UFID for a flow, which is stored with the flow
> and inserted into a separate table for lookup, in addition to the
> standard flow table. Flows created using a UFID must be fetched or
> deleted using the UFID.
>
> All flow dump operations may now be made more terse with OVS_UFID_F_*
> flags. For example, the OVS_UFID_F_OMIT_KEY flag allows responses to
> omit the flow key from a datapath operation if the flow has a
> corresponding UFID. This significantly reduces the time spent assembling
> and transacting netlink messages. With all OVS_UFID_F_OMIT_* flags
> enabled, the datapath only returns the UFID and statistics for each flow
> during flow dump, increasing ovs-vswitchd revalidator performance by up
> to 50%.
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> CC: Pravin B Shelar <pshelar@nicira.com>
> CC: netdev@vger.kernel.org
> ---
> v10: Ignore flow_key in requests if UFID is specified.
> Only allow UFID flows to be indexed by UFID.
> Only allow non-UFID flows to be indexed by unmasked flow key.
> Unite the unmasked_key and ufid+ufid_hash in 'struct sw_flow'.
> Don't periodically rehash the UFID table.
> Resize the UFID table independently from the flow table.
> Modify table_destroy() to iterate once and delete from both tables.
> Fix UFID memory leak in flow_free().
> Remove kernel-only UFIDs for non-UFID cases.
> Rename "OVS_UFID_F_SKIP_*" -> "OVS_UFID_F_OMIT_*"
> Update documentation.
> Rebase.
> v9: No change.
> v8: Rename UID -> UFID "unique flow identifier".
> Fix null dereference when adding flow without uid or mask.
> If UFID and not match are specified, and lookup fails, return ENOENT.
> Rebase.
> v7: Remove OVS_DP_F_INDEX_BY_UID.
> Rework UID serialisation for variable-length UID.
> Log error if uid not specified and OVS_UID_F_SKIP_KEY is set.
> Rebase against "probe" logging changes.
> v6: Fix documentation for supporting UIDs between 32-128 bits.
> Minor style fixes.
> Rebase.
> v5: No change.
> v4: Fix memory leaks.
> Log when triggering the older userspace issue above.
> v3: Initial post.
> ---
> datapath/README.md | 13 ++
> datapath/datapath.c | 248 +++++++++++++++------
> datapath/flow.h | 20 +-
> datapath/flow_netlink.c | 35 +++
> datapath/flow_netlink.h | 1 +
> datapath/flow_table.c | 214 ++++++++++++++----
> datapath/flow_table.h | 8 +
> datapath/linux/compat/include/linux/openvswitch.h | 30 +++
> 8 files changed, 453 insertions(+), 116 deletions(-)
>
> diff --git a/datapath/README.md b/datapath/README.md
> index a8effa3..9c03a2b 100644
> --- a/datapath/README.md
> +++ b/datapath/README.md
> @@ -131,6 +131,19 @@ performs best-effort detection of overlapping wildcarded flows and may reject
> some but not all of them. However, this behavior may change in future versions.
>
>
> +Unique flow identifiers
> +-----------------------
> +
> +An alternative to using the original match portion of a key as the handle for
> +flow identification is a unique flow identifier, or "UFID". UFIDs are optional
> +for both the kernel and user space program.
> +
> +User space programs that support UFID are expected to provide it during flow
> +setup in addition to the flow, then refer to the flow using the UFID for all
> +future operations. The kernel is not required to index flows by the original
> +flow key if a UFID is specified.
> +
> +
> Basic rule for evolving flow keys
> ---------------------------------
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index a898584..c14d834 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -671,11 +671,18 @@ static void get_dp_stats(const struct datapath *dp, struct ovs_dp_stats *stats,
> }
> }
>
> -static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
> +static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts,
> + const struct sw_flow_id *sfid)
> {
> + size_t sfid_len = 0;
> +
> + if (sfid && sfid->ufid_len)
> + sfid_len = nla_total_size(sfid->ufid_len);
> +
> return NLMSG_ALIGN(sizeof(struct ovs_header))
> + nla_total_size(ovs_key_attr_size()) /* OVS_FLOW_ATTR_KEY */
> + nla_total_size(ovs_key_attr_size()) /* OVS_FLOW_ATTR_MASK */
> + + sfid_len /* OVS_FLOW_ATTR_UFID */
> + nla_total_size(sizeof(struct ovs_flow_stats)) /* OVS_FLOW_ATTR_STATS */
> + nla_total_size(1) /* OVS_FLOW_ATTR_TCP_FLAGS */
> + nla_total_size(8) /* OVS_FLOW_ATTR_USED */
> @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
>
> /* Called with ovs_mutex or RCU read lock. */
> static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
> - struct sk_buff *skb)
> + struct sk_buff *skb, u32 ufid_flags)
> {
> struct nlattr *nla;
> int err;
>
> - /* Fill flow key. */
> - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> - if (!nla)
> - return -EMSGSIZE;
> -
> - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key, skb,
> - false);
> - if (err)
> - return err;
> -
> - nla_nest_end(skb, nla);
> + /* Fill flow key. If userspace didn't specify a UFID, then ignore the
> + * OMIT_KEY flag. */
> + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
> + !flow->index_by_ufid) {
I am not sure about this check, userspace needs to send atleast ufid
or the unmasked key as id for flow. otherwise we shld flag error. Here
we can serialize flow->key.
There could be another function which takes care of flow-id
serialization where we serialize use ufid or unmasked key as flow id.
Lets group ufid and unmasked key together rather than masked key and
unmasked key which are not related.
> + nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> + if (!nla)
> + return -EMSGSIZE;
> +
> + if (flow->index_by_ufid)
> + err = ovs_nla_put_flow(&flow->mask->key, &flow->key,
> + skb, false);
> + else
> + err = ovs_nla_put_flow(&flow->index.unmasked_key,
> + &flow->index.unmasked_key, skb,
> + false);
> + if (err)
> + return err;
> + nla_nest_end(skb, nla);
> + }
>
> /* Fill flow mask. */
> - nla = nla_nest_start(skb, OVS_FLOW_ATTR_MASK);
> - if (!nla)
> - return -EMSGSIZE;
> + if (!(ufid_flags & OVS_UFID_F_OMIT_MASK)) {
> + nla = nla_nest_start(skb, OVS_FLOW_ATTR_MASK);
> + if (!nla)
> + return -EMSGSIZE;
>
> - err = ovs_nla_put_flow(&flow->key, &flow->mask->key, skb, true);
> - if (err)
> - return err;
> + err = ovs_nla_put_flow(&flow->key, &flow->mask->key, skb, true);
> + if (err)
> + return err;
> + nla_nest_end(skb, nla);
> + }
>
> - nla_nest_end(skb, nla);
> return 0;
> }
>
> @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct sw_flow *flow,
> }
>
> /* Called with ovs_mutex or RCU read lock. */
> +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
> + struct sk_buff *skb)
> +{
> + struct nlattr *start;
> + const struct sw_flow_id *sfid;
> +
> + if (!flow->index_by_ufid)
> + return 0;
> +
> + sfid = &flow->index.ufid;
> + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
> + if (start) {
> + int err;
> +
> + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
> + sfid->ufid);
> + if (err)
> + return err;
> + nla_nest_end(skb, start);
> + } else
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
Can you change this function according to comments above?
> +/* Called with ovs_mutex or RCU read lock. */
> static int ovs_flow_cmd_fill_actions(const struct sw_flow *flow,
> struct sk_buff *skb, int skb_orig_len)
> {
> @@ -782,7 +825,7 @@ static int ovs_flow_cmd_fill_actions(const struct sw_flow *flow,
> /* Called with ovs_mutex or RCU read lock. */
> static int ovs_flow_cmd_fill_info(const struct sw_flow *flow, int dp_ifindex,
> struct sk_buff *skb, u32 portid,
> - u32 seq, u32 flags, u8 cmd)
> + u32 seq, u32 flags, u8 cmd, u32 ufid_flags)
> {
> const int skb_orig_len = skb->len;
> struct ovs_header *ovs_header;
> @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct sw_flow *flow, int dp_ifindex,
>
> ovs_header->dp_ifindex = dp_ifindex;
>
> - err = ovs_flow_cmd_fill_match(flow, skb);
> + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
> if (err)
> goto error;
>
> - err = ovs_flow_cmd_fill_stats(flow, skb);
> + err = ovs_flow_cmd_fill_ufid(flow, skb);
> if (err)
> goto error;
Flow ID should go first in the netlink msg.
>
> - err = ovs_flow_cmd_fill_actions(flow, skb, skb_orig_len);
> + err = ovs_flow_cmd_fill_stats(flow, skb);
> if (err)
> goto error;
>
> + if (!(ufid_flags & OVS_UFID_F_OMIT_ACTIONS)) {
> + err = ovs_flow_cmd_fill_actions(flow, skb, skb_orig_len);
> + if (err)
> + goto error;
> + }
> +
> return genlmsg_end(skb, ovs_header);
>
> error:
> @@ -816,6 +865,7 @@ error:
>
> /* May not be called with RCU read lock. */
> static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *acts,
> + const struct sw_flow_id *sfid,
> struct genl_info *info,
> bool always)
> {
> @@ -825,30 +875,36 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
> GROUP_ID(&ovs_dp_flow_multicast_group)))
> return NULL;
>
> - skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info, GFP_KERNEL);
> + skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts, sfid), info,
> + GFP_KERNEL);
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> return skb;
> }
>
> +static const struct sw_flow_id *flow_get_ufid(const struct sw_flow *flow)
> +{
> + return flow->index_by_ufid ? &flow->index.ufid : NULL;
> +}
> +
> /* Called with ovs_mutex. */
> static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
> int dp_ifindex,
> struct genl_info *info, u8 cmd,
> - bool always)
> + bool always, u32 ufid_flags)
> {
> struct sk_buff *skb;
> int retval;
>
> - skb = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts), info,
> - always);
> + skb = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts),
> + flow_get_ufid(flow), info, always);
> if (IS_ERR_OR_NULL(skb))
> return skb;
>
> retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
> info->snd_portid, info->snd_seq, 0,
> - cmd);
> + cmd, ufid_flags);
> BUG_ON(retval < 0);
> return skb;
> }
> @@ -863,6 +919,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> struct datapath *dp;
> struct sw_flow_actions *acts;
> struct sw_flow_match match;
> + struct sw_flow_id sfid;
> + u32 ufid_flags;
> int error;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> @@ -887,13 +945,20 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> }
>
> /* Extract key. */
> - ovs_match_init(&match, &new_flow->unmasked_key, &mask);
> + ovs_match_init(&match, &new_flow->index.unmasked_key, &mask);
> error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto err_kfree_flow;
>
> - ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
> + ovs_flow_mask_key(&new_flow->key, &new_flow->index.unmasked_key, &mask);
> +
> + /* Extract ufid. */
> + error = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &sfid, &ufid_flags);
> + if (!error)
> + error = ovs_flow_ufid(new_flow, &sfid);
> + if (error)
> + goto err_kfree_flow;
>
> /* Validate actions. */
> error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
> @@ -903,7 +968,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> goto err_kfree_flow;
> }
>
> - reply = ovs_flow_cmd_alloc_info(acts, info, false);
> + reply = ovs_flow_cmd_alloc_info(acts, &sfid, info, false);
> if (IS_ERR(reply)) {
> error = PTR_ERR(reply);
> goto err_kfree_acts;
> @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> error = -ENODEV;
> goto err_unlock_ovs;
> }
> +
> /* Check if this is a duplicate flow */
> - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
> + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
Need to check for ufid table to find duplicate ufid entry here.
> if (likely(!flow)) {
> rcu_assign_pointer(new_flow->sf_acts, acts);
>
> @@ -932,7 +998,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_NEW);
> + OVS_FLOW_CMD_NEW,
> + ufid_flags);
> BUG_ON(error < 0);
> }
> ovs_unlock();
> @@ -950,14 +1017,16 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> error = -EEXIST;
> goto err_unlock_ovs;
> }
> - /* The unmasked key has to be the same for flow updates. */
> - if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> - /* Look for any overlapping flow. */
> + /* The flow identifier has to be the same for flow updates. */
> + if (sfid.ufid_len) {
> + if (unlikely(!ovs_flow_cmp_ufid(flow, &sfid)))
> + flow = NULL;
> + } else if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> - if (!flow) {
> - error = -ENOENT;
> - goto err_unlock_ovs;
> - }
> + }
> + if (unlikely(!flow)) {
> + error = -ENOENT;
> + goto err_unlock_ovs;
> }
> /* Update actions. */
> old_acts = ovsl_dereference(flow->sf_acts);
> @@ -968,7 +1037,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_NEW);
> + OVS_FLOW_CMD_NEW,
> + ufid_flags);
> BUG_ON(error < 0);
> }
> ovs_unlock();
> @@ -1018,30 +1088,36 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> struct nlattr **a = info->attrs;
> struct ovs_header *ovs_header = info->userhdr;
> struct sw_flow_key key;
> - struct sw_flow *flow;
> + struct sw_flow *flow = NULL;
> struct sw_flow_mask mask;
> struct sk_buff *reply = NULL;
> struct datapath *dp;
> struct sw_flow_actions *old_acts = NULL, *acts = NULL;
> struct sw_flow_match match;
> + struct sw_flow_id ufid;
> + u32 ufid_flags;
> int error;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> - /* Extract key. */
> - error = -EINVAL;
> - if (!a[OVS_FLOW_ATTR_KEY]) {
> - OVS_NLERR(log, "Flow key attribute not present in set flow.");
> + /* Extract ufid/key. */
> + error = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &ufid,
> + &ufid_flags);
> + if (error)
> goto error;
> + if (a[OVS_FLOW_ATTR_KEY]) {
> + ovs_match_init(&match, &key, &mask);
> + error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> + a[OVS_FLOW_ATTR_MASK], log);
> + } else if (!ufid.ufid_len) {
> + OVS_NLERR(log, "Flow key attribute not present in set flow.\n");
> + error = -EINVAL;
> }
> -
> - ovs_match_init(&match, &key, &mask);
> - error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> - a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto error;
>
> /* Validate actions. */
> - if (a[OVS_FLOW_ATTR_ACTIONS]) {
> + if (a[OVS_FLOW_ATTR_ACTIONS] && a[OVS_FLOW_ATTR_KEY] &&
> + a[OVS_FLOW_ATTR_MASK]) {
> acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask,
> log);
> if (IS_ERR(acts)) {
> @@ -1050,7 +1126,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> }
>
> /* Can allocate before locking if have acts. */
> - reply = ovs_flow_cmd_alloc_info(acts, info, false);
> + reply = ovs_flow_cmd_alloc_info(acts, &ufid, info, false);
> if (IS_ERR(reply)) {
> error = PTR_ERR(reply);
> goto err_kfree_acts;
> @@ -1064,7 +1140,10 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> goto err_unlock_ovs;
> }
> /* Check that the flow exists. */
> - flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (ufid.ufid_len)
> + flow = ovs_flow_tbl_lookup_ufid(&dp->table, &ufid);
> + else
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> if (unlikely(!flow)) {
> error = -ENOENT;
> goto err_unlock_ovs;
> @@ -1080,13 +1159,15 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_NEW);
> + OVS_FLOW_CMD_NEW,
> + ufid_flags);
> BUG_ON(error < 0);
> }
> } else {
> /* Could not alloc without acts before locking. */
> reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> - info, OVS_FLOW_CMD_NEW, false);
> + info, OVS_FLOW_CMD_NEW, false,
> + ufid_flags);
> if (unlikely(IS_ERR(reply))) {
> error = PTR_ERR(reply);
> goto err_unlock_ovs;
> @@ -1123,17 +1204,22 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
> struct sw_flow *flow;
> struct datapath *dp;
> struct sw_flow_match match;
> + struct sw_flow_id ufid;
> + u32 ufid_flags;
> int err;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> - if (!a[OVS_FLOW_ATTR_KEY]) {
> + err = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &ufid, &ufid_flags);
> + if (err)
> + return err;
> + if (a[OVS_FLOW_ATTR_KEY]) {
> + ovs_match_init(&match, &key, NULL);
> + err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL, log);
> + } else if (!ufid.ufid_len) {
> OVS_NLERR(log,
> - "Flow get message rejected, Key attribute missing.");
> - return -EINVAL;
> + "Flow get message rejected, Key attribute missing.\n");
> + err = -EINVAL;
> }
> -
> - ovs_match_init(&match, &key, NULL);
> - err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL, log);
> if (err)
> return err;
>
> @@ -1144,14 +1230,17 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
> goto unlock;
> }
>
> - flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (ufid.ufid_len)
> + flow = ovs_flow_tbl_lookup_ufid(&dp->table, &ufid);
> + else
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> if (!flow) {
> err = -ENOENT;
> goto unlock;
> }
>
> reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, info,
> - OVS_FLOW_CMD_NEW, true);
> + OVS_FLOW_CMD_NEW, true, ufid_flags);
> if (IS_ERR(reply)) {
> err = PTR_ERR(reply);
> goto unlock;
> @@ -1170,13 +1259,18 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> struct ovs_header *ovs_header = info->userhdr;
> struct sw_flow_key key;
> struct sk_buff *reply;
> - struct sw_flow *flow;
> + struct sw_flow *flow = NULL;
> struct datapath *dp;
> struct sw_flow_match match;
> + struct sw_flow_id ufid;
> + u32 ufid_flags;
> int err;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> - if (likely(a[OVS_FLOW_ATTR_KEY])) {
> + err = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &ufid, &ufid_flags);
> + if (err)
> + return err;
> + if (a[OVS_FLOW_ATTR_KEY]) {
> ovs_match_init(&match, &key, NULL);
> err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL,
> log);
> @@ -1191,12 +1285,15 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> goto unlock;
> }
>
> - if (unlikely(!a[OVS_FLOW_ATTR_KEY])) {
> + if (unlikely(!a[OVS_FLOW_ATTR_KEY] && !ufid.ufid_len)) {
> err = ovs_flow_tbl_flush(&dp->table);
> goto unlock;
> }
>
> - flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (ufid.ufid_len)
> + flow = ovs_flow_tbl_lookup_ufid(&dp->table, &ufid);
> + else
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> if (unlikely(!flow)) {
> err = -ENOENT;
> goto unlock;
> @@ -1206,7 +1303,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> ovs_unlock();
>
> reply = ovs_flow_cmd_alloc_info(rcu_dereference_raw(flow->sf_acts),
> - info, false);
> + flow_get_ufid(flow), info, false);
>
> if (likely(reply)) {
> if (likely(!IS_ERR(reply))) {
> @@ -1214,7 +1311,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> err = ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_DEL);
> + OVS_FLOW_CMD_DEL,
> + ufid_flags);
> rcu_read_unlock();
> BUG_ON(err < 0);
> ovs_notify(&dp_flow_genl_family, &ovs_dp_flow_multicast_group, reply, info);
> @@ -1235,8 +1333,15 @@ unlock:
> static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
> {
> struct ovs_header *ovs_header = genlmsg_data(nlmsg_data(cb->nlh));
> + struct nlattr *nla, *ufid;
> struct table_instance *ti;
> struct datapath *dp;
> + u32 ufid_flags = 0;
> +
> + nla = nlmsg_attrdata(cb->nlh, sizeof(*ovs_header));
> + ufid = nla_find_nested(nla, OVS_FLOW_ATTR_UFID);
> + if (ufid && ovs_nla_get_ufid(ufid, NULL, &ufid_flags))
> + OVS_NLERR(true, "Error occurred parsing UFID flags on dump");
>
> rcu_read_lock();
> dp = get_dp_rcu(sock_net(skb->sk), ovs_header->dp_ifindex);
> @@ -1259,7 +1364,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
> if (ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, skb,
> NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq, NLM_F_MULTI,
> - OVS_FLOW_CMD_NEW) < 0)
> + OVS_FLOW_CMD_NEW, ufid_flags) < 0)
> break;
>
> cb->args[0] = bucket;
> @@ -1275,6 +1380,7 @@ static const struct nla_policy flow_policy[OVS_FLOW_ATTR_MAX + 1] = {
> [OVS_FLOW_ATTR_ACTIONS] = { .type = NLA_NESTED },
> [OVS_FLOW_ATTR_CLEAR] = { .type = NLA_FLAG },
> [OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },
> + [OVS_FLOW_ATTR_UFID] = { .type = NLA_NESTED },
> };
>
> static struct genl_ops dp_flow_genl_ops[] = {
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 2bbf789..736e0eb 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -196,6 +196,13 @@ struct sw_flow_match {
> struct sw_flow_mask *mask;
> };
>
> +struct sw_flow_id {
> + struct hlist_node node[2];
> + u32 hash;
> + u8 *ufid;
> + u8 ufid_len;
> +};
> +
Lets make ufid array of size 256, we can reject any key greater than
this. current patch does not support key greater than 256 anyways.
> struct sw_flow_actions {
> struct rcu_head rcu;
> u32 actions_len;
> @@ -212,13 +219,20 @@ struct flow_stats {
>
> struct sw_flow {
> struct rcu_head rcu;
> - struct hlist_node hash_node[2];
> - u32 hash;
> + struct {
> + struct hlist_node node[2];
> + u32 hash;
> + } flow_hash;
This change does not look related to this work.
> int stats_last_writer; /* NUMA-node id of the last writer on
> * 'stats[0]'.
> */
> struct sw_flow_key key;
> - struct sw_flow_key unmasked_key;
> + bool index_by_ufid; /* Which of the below that userspace
> + uses to index this flow. */
> + union {
> + struct sw_flow_key unmasked_key;
> + struct sw_flow_id ufid;
> + } index;
Rather than storing ufid or unmasked key inside struct flow we can
keep pointer to these objects, that will save some memory.
> struct sw_flow_mask *mask;
> struct sw_flow_actions __rcu *sf_acts;
> struct flow_stats __rcu *stats[]; /* One for each NUMA node. First one
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index c1c29f5..7927462 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1094,6 +1094,41 @@ free_newmask:
> return err;
> }
>
> +int ovs_nla_get_ufid(const struct nlattr *attr, struct sw_flow_id *sfid,
> + u32 *flags)
> +{
> + static const struct nla_policy ovs_ufid_policy[OVS_UFID_ATTR_MAX + 1] = {
> + [OVS_UFID_ATTR_FLAGS] = { .type = NLA_U32 },
> + [OVS_UFID_ATTR_ID] = { .len = sizeof(u32) },
> + };
> + const struct nlattr *a[OVS_UFID_ATTR_MAX + 1];
> + int err;
> +
> + if (sfid) {
> + sfid->ufid = NULL;
> + sfid->ufid_len = 0;
> + }
> + if (flags)
> + *flags = 0;
> + if (!attr)
> + return 0;
> +
> + err = nla_parse_nested((struct nlattr **)a, OVS_UFID_ATTR_MAX, attr,
> + ovs_ufid_policy);
> + if (err)
> + return err;
> +
> + if (sfid && a[OVS_UFID_ATTR_ID]) {
> + sfid->ufid = nla_data(a[OVS_UFID_ATTR_ID]);
> + sfid->ufid_len = nla_len(a[OVS_UFID_ATTR_ID]);
> + }
> +
> + if (flags && a[OVS_UFID_ATTR_FLAGS])
> + *flags = nla_get_u32(a[OVS_UFID_ATTR_FLAGS]);
> +
> + return 0;
> +}
> +
> /**
> * ovs_nla_get_flow_metadata - parses Netlink attributes into a flow key.
> * @key: Receives extracted in_port, priority, tun_key and skb_mark.
> diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h
> index fde7616..9a14ad9 100644
> --- a/datapath/flow_netlink.h
> +++ b/datapath/flow_netlink.h
> @@ -52,6 +52,7 @@ int ovs_nla_get_match(struct sw_flow_match *, const struct nlattr *key,
> const struct nlattr *mask, bool log);
> int ovs_nla_put_egress_tunnel_key(struct sk_buff *,
> const struct ovs_tunnel_info *);
> +int ovs_nla_get_ufid(const struct nlattr *, struct sw_flow_id *, u32 *flags);
>
> int ovs_nla_copy_actions(const struct nlattr *attr,
> const struct sw_flow_key *key,
> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index ad410fd..03e7040 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> @@ -92,6 +92,7 @@ struct sw_flow *ovs_flow_alloc(void)
>
> flow->sf_acts = NULL;
> flow->mask = NULL;
> + flow->index_by_ufid = false;
> flow->stats_last_writer = NUMA_NO_NODE;
>
> /* Initialize the default stat node. */
> @@ -146,6 +147,8 @@ static void flow_free(struct sw_flow *flow)
> {
> int node;
>
> + if (flow->index_by_ufid)
> + kfree(flow->index.ufid.ufid);
> kfree(rcu_dereference_raw(flow->sf_acts));
> for_each_node(node)
> if (flow->stats[node])
> @@ -265,7 +268,7 @@ static int tbl_mask_array_realloc(struct flow_table *tbl, int size)
>
> int ovs_flow_tbl_init(struct flow_table *table)
> {
> - struct table_instance *ti;
> + struct table_instance *ti, *ufid_ti;
> struct mask_array *ma;
>
> table->mask_cache = __alloc_percpu(sizeof(struct mask_cache_entry) *
> @@ -277,16 +280,24 @@ int ovs_flow_tbl_init(struct flow_table *table)
> if (!ma)
> goto free_mask_cache;
>
> + ufid_ti = table_instance_alloc(TBL_MIN_BUCKETS);
> + if (!ufid_ti)
> + goto free_mask_array;
> +
> ti = table_instance_alloc(TBL_MIN_BUCKETS);
> if (!ti)
> - goto free_mask_array;
> + goto free_ti;
>
> rcu_assign_pointer(table->ti, ti);
> + rcu_assign_pointer(table->ufid_ti, ufid_ti);
> rcu_assign_pointer(table->mask_array, ma);
> - table->last_rehash = jiffies;
> table->count = 0;
> + table->ufid_count = 0;
> + table->last_rehash = jiffies;
> return 0;
>
> +free_ti:
> + __table_instance_destroy(ufid_ti);
> free_mask_array:
> kfree(ma);
> free_mask_cache:
> @@ -301,13 +312,16 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
> __table_instance_destroy(ti);
> }
>
> -static void table_instance_destroy(struct table_instance *ti, bool deferred)
> +static void table_instance_destroy(struct table_instance *ti,
> + struct table_instance *ufid_ti,
> + bool deferred)
> {
> int i;
>
> if (!ti)
> return;
>
> + BUG_ON(!ufid_ti);
> if (ti->keep_flows)
> goto skip_flows;
>
> @@ -316,18 +330,24 @@ static void table_instance_destroy(struct table_instance *ti, bool deferred)
> struct hlist_head *head = flex_array_get(ti->buckets, i);
> struct hlist_node *n;
> int ver = ti->node_ver;
> + int ufid_ver = ufid_ti->node_ver;
>
> - hlist_for_each_entry_safe(flow, n, head, hash_node[ver]) {
> - hlist_del_rcu(&flow->hash_node[ver]);
> + hlist_for_each_entry_safe(flow, n, head, flow_hash.node[ver]) {
> + hlist_del_rcu(&flow->flow_hash.node[ver]);
> + if (flow->index_by_ufid)
> + hlist_del_rcu(&flow->index.ufid.node[ufid_ver]);
> ovs_flow_free(flow, deferred);
> }
> }
>
> skip_flows:
> - if (deferred)
> + if (deferred) {
> call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
> - else
> + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
> + } else {
> __table_instance_destroy(ti);
> + __table_instance_destroy(ufid_ti);
> + }
> }
>
> /* No need for locking this function is called from RCU callback or
> @@ -336,10 +356,11 @@ skip_flows:
> void ovs_flow_tbl_destroy(struct flow_table *table)
> {
> struct table_instance *ti = rcu_dereference_raw(table->ti);
> + struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
>
> free_percpu(table->mask_cache);
> - kfree(rcu_dereference_raw(table->mask_array));
> - table_instance_destroy(ti, false);
> + kfree((struct mask_array __force *)table->mask_array);
> + table_instance_destroy(ti, ufid_ti, false);
> }
>
> struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
> @@ -354,7 +375,7 @@ struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
> while (*bucket < ti->n_buckets) {
> i = 0;
> head = flex_array_get(ti->buckets, *bucket);
> - hlist_for_each_entry_rcu(flow, head, hash_node[ver]) {
> + hlist_for_each_entry_rcu(flow, head, flow_hash.node[ver]) {
> if (i < *last) {
> i++;
> continue;
> @@ -380,12 +401,21 @@ static void table_instance_insert(struct table_instance *ti, struct sw_flow *flo
> {
> struct hlist_head *head;
>
> - head = find_bucket(ti, flow->hash);
> - hlist_add_head_rcu(&flow->hash_node[ti->node_ver], head);
> + head = find_bucket(ti, flow->flow_hash.hash);
> + hlist_add_head_rcu(&flow->flow_hash.node[ti->node_ver], head);
> +}
> +
> +static void ufid_table_instance_insert(struct table_instance *ti,
> + struct sw_flow *flow)
> +{
> + struct hlist_head *head;
> +
> + head = find_bucket(ti, flow->index.ufid.hash);
> + hlist_add_head_rcu(&flow->index.ufid.node[ti->node_ver], head);
> }
>
> static void flow_table_copy_flows(struct table_instance *old,
> - struct table_instance *new)
> + struct table_instance *new, bool ufid)
> {
> int old_ver;
> int i;
> @@ -400,42 +430,69 @@ static void flow_table_copy_flows(struct table_instance *old,
>
> head = flex_array_get(old->buckets, i);
>
> - hlist_for_each_entry(flow, head, hash_node[old_ver])
> - table_instance_insert(new, flow);
> + if (ufid)
> + hlist_for_each_entry(flow, head,
> + index.ufid.node[old_ver])
> + ufid_table_instance_insert(new, flow);
> + else
> + hlist_for_each_entry(flow, head, flow_hash.node[old_ver])
> + table_instance_insert(new, flow);
> }
>
> old->keep_flows = true;
> }
>
> -static struct table_instance *table_instance_rehash(struct table_instance *ti,
> - int n_buckets)
> +static int flow_table_instance_alloc(struct table_instance **ti,
> + int n_buckets)
> {
> struct table_instance *new_ti;
>
> new_ti = table_instance_alloc(n_buckets);
> if (!new_ti)
> - return NULL;
> + return -ENOMEM;
>
> - flow_table_copy_flows(ti, new_ti);
> + *ti = new_ti;
> + return 0;
> +}
> +
> +static struct table_instance *flow_table_rehash(struct table_instance *old_ti,
> + int n_buckets, bool ufid)
> +{
> + struct table_instance *new_ti;
> + int err;
> +
> + err = flow_table_instance_alloc(&new_ti, n_buckets);
> + if (err)
> + return NULL;
> + flow_table_copy_flows(old_ti, new_ti, ufid);
>
> return new_ti;
> }
>
> int ovs_flow_tbl_flush(struct flow_table *flow_table)
> {
> - struct table_instance *old_ti;
> - struct table_instance *new_ti;
> + struct table_instance *old_ti, *new_ti, *old_ufid_ti;
> + struct table_instance *new_ufid_ti = NULL;
> + int err;
>
> old_ti = ovsl_dereference(flow_table->ti);
> - new_ti = table_instance_alloc(TBL_MIN_BUCKETS);
> - if (!new_ti)
> - return -ENOMEM;
> + old_ufid_ti = ovsl_dereference(flow_table->ufid_ti);
> + err = flow_table_instance_alloc(&new_ti, TBL_MIN_BUCKETS);
> + if (err)
> + return err;
> + err = flow_table_instance_alloc(&new_ufid_ti, TBL_MIN_BUCKETS);
> + if (err) {
> + __table_instance_destroy(new_ti);
> + return err;
> + }
>
> rcu_assign_pointer(flow_table->ti, new_ti);
> + rcu_assign_pointer(flow_table->ufid_ti, new_ufid_ti);
> flow_table->last_rehash = jiffies;
> flow_table->count = 0;
> + flow_table->ufid_count = 0;
>
> - table_instance_destroy(old_ti, true);
> + table_instance_destroy(old_ti, old_ufid_ti, true);
> return 0;
> }
>
> @@ -489,7 +546,8 @@ bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
> int key_start = flow_key_start(key);
> int key_end = match->range.end;
>
> - return cmp_key(&flow->unmasked_key, key, key_start, key_end);
> + BUG_ON(flow->index_by_ufid);
> + return cmp_key(&flow->index.unmasked_key, key, key_start, key_end);
> }
>
> static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
> @@ -508,10 +566,9 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
> hash = flow_hash(&masked_key, key_start, key_end);
> head = find_bucket(ti, hash);
> (*n_mask_hit)++;
> - hlist_for_each_entry_rcu(flow, head, hash_node[ti->node_ver]) {
> - if (flow->mask == mask && flow->hash == hash &&
> - flow_cmp_masked_key(flow, &masked_key,
> - key_start, key_end))
> + hlist_for_each_entry_rcu(flow, head, flow_hash.node[ti->node_ver]) {
> + if (flow->mask == mask && flow->flow_hash.hash == hash &&
> + flow_cmp_masked_key(flow, &masked_key, key_start, key_end))
> return flow;
> }
> return NULL;
> @@ -644,7 +701,40 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> if (!mask)
> continue;
> flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
> - if (flow && ovs_flow_cmp_unmasked_key(flow, match))
> + if (flow && !flow->index_by_ufid &&
> + ovs_flow_cmp_unmasked_key(flow, match))
> + return flow;
> + }
> + return NULL;
> +}
> +
> +static u32 ufid_hash(const struct sw_flow_id *sfid)
> +{
> + return arch_fast_hash(sfid->ufid, sfid->ufid_len, 0);
> +}
> +
> +bool ovs_flow_cmp_ufid(const struct sw_flow *flow,
> + const struct sw_flow_id *sfid)
> +{
> + if (flow->index.ufid.ufid_len != sfid->ufid_len)
> + return false;
> +
> + return !memcmp(flow->index.ufid.ufid, sfid->ufid, sfid->ufid_len);
> +}
> +
> +struct sw_flow *ovs_flow_tbl_lookup_ufid(struct flow_table *tbl,
> + const struct sw_flow_id *ufid)
> +{
> + struct table_instance *ti = rcu_dereference_ovsl(tbl->ufid_ti);
> + struct sw_flow *flow;
> + struct hlist_head *head;
> + u32 hash;
> +
> + hash = ufid_hash(ufid);
> + head = find_bucket(ti, hash);
> + hlist_for_each_entry_rcu(flow, head, index.ufid.node[ti->node_ver]) {
> + if (flow->index.ufid.hash == hash &&
> + ovs_flow_cmp_ufid(flow, ufid))
> return flow;
> }
> return NULL;
> @@ -658,9 +748,10 @@ int ovs_flow_tbl_num_masks(const struct flow_table *table)
> return ma->count;
> }
>
> -static struct table_instance *table_instance_expand(struct table_instance *ti)
> +static struct table_instance *flow_table_expand(struct table_instance *old_ti,
> + bool ufid)
> {
> - return table_instance_rehash(ti, ti->n_buckets * 2);
> + return flow_table_rehash(old_ti, old_ti->n_buckets * 2, ufid);
> }
>
> static void tbl_mask_array_delete_mask(struct mask_array *ma,
> @@ -710,10 +801,15 @@ static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
> void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow)
> {
> struct table_instance *ti = ovsl_dereference(table->ti);
> + struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
>
> BUG_ON(table->count == 0);
> - hlist_del_rcu(&flow->hash_node[ti->node_ver]);
> + hlist_del_rcu(&flow->flow_hash.node[ti->node_ver]);
> table->count--;
> + if (flow->index_by_ufid) {
> + hlist_del_rcu(&flow->index.ufid.node[ufid_ti->node_ver]);
> + table->ufid_count--;
> + }
>
> /* RCU delete the mask. 'flow->mask' is not NULLed, as it should be
> * accessible as long as the RCU read lock is held.
> @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
> int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
> const struct sw_flow_mask *mask)
> {
> - struct table_instance *new_ti = NULL;
> - struct table_instance *ti;
> + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
> + struct table_instance *ti, *ufid_ti = NULL;
> int err;
>
> err = flow_mask_insert(table, flow, mask);
> if (err)
> return err;
>
> - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
> - flow->mask->range.end);
> + flow->flow_hash.hash = flow_hash(&flow->key, flow->mask->range.start,
> + flow->mask->range.end);
> ti = ovsl_dereference(table->ti);
> table_instance_insert(ti, flow);
> table->count++;
> + if (flow->index_by_ufid) {
> + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
> + ufid_ti = ovsl_dereference(table->ufid_ti);
> + ufid_table_instance_insert(ufid_ti, flow);
> + table->ufid_count++;
> + }
>
> /* Expand table, if necessary, to make room. */
> if (table->count > ti->n_buckets)
> - new_ti = table_instance_expand(ti);
> + new_ti = flow_table_expand(ti, false);
> else if (time_after(jiffies, table->last_rehash + REHASH_INTERVAL))
> - new_ti = table_instance_rehash(ti, ti->n_buckets);
> + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
> + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
> + new_ufid_ti = flow_table_expand(ufid_ti, true);
>
> if (new_ti) {
> rcu_assign_pointer(table->ti, new_ti);
> - table_instance_destroy(ti, true);
> + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
> table->last_rehash = jiffies;
> }
> + if (new_ufid_ti) {
> + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
> + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
> + }
> + return 0;
> +}
Insert function can be simplified by first updating flow-table and
then updating ufid table.
> +
> +/* Initializes 'flow->ufid'. */
> +int ovs_flow_ufid(struct sw_flow *flow, const struct sw_flow_id *src)
> +{
> + if (src->ufid_len) {
> + struct sw_flow_id *dst = &flow->index.ufid;
> +
> + /* Use userspace-specified flow-id. */
> + dst->ufid = kmalloc(src->ufid_len, GFP_KERNEL);
> + if (!dst->ufid)
> + return -ENOMEM;
> +
> + memcpy(dst->ufid, src->ufid, src->ufid_len);
> + dst->ufid_len = src->ufid_len;
> + flow->index_by_ufid = true;
> + } else {
> + /* Don't index by UFID. */
> + flow->index_by_ufid = false;
> + }
> +
> return 0;
> }
>
> diff --git a/datapath/flow_table.h b/datapath/flow_table.h
> index 80c01a3..e05b59c 100644
> --- a/datapath/flow_table.h
> +++ b/datapath/flow_table.h
> @@ -60,8 +60,10 @@ struct flow_table {
> struct table_instance __rcu *ti;
> struct mask_cache_entry __percpu *mask_cache;
> struct mask_array __rcu *mask_array;
> + struct table_instance __rcu *ufid_ti;
> unsigned long last_rehash;
> unsigned int count;
> + unsigned int ufid_count;
> };
>
> extern struct kmem_cache *flow_stats_cache;
> @@ -91,9 +93,15 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
> const struct sw_flow_key *);
> struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> const struct sw_flow_match *match);
> +struct sw_flow *ovs_flow_tbl_lookup_ufid(struct flow_table *,
> + const struct sw_flow_id *);
> +
> bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
> const struct sw_flow_match *match);
> +bool ovs_flow_cmp_ufid(const struct sw_flow *flow,
> + const struct sw_flow_id *sfid);
>
> void ovs_flow_mask_key(struct sw_flow_key *dst, const struct sw_flow_key *src,
> const struct sw_flow_mask *mask);
> +int ovs_flow_ufid(struct sw_flow *flow, const struct sw_flow_id *src);
> #endif /* flow_table.h */
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index c8fa66e..7d759a4 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -471,6 +471,9 @@ struct ovs_key_nd {
> * a wildcarded match. Omitting attribute is treated as wildcarding all
> * corresponding fields. Optional for all requests. If not present,
> * all flow key bits are exact match bits.
> + * @OVS_FLOW_ATTR_UFID: Nested %OVS_UFID_ATTR_* attributes specifying unique
> + * identifiers for flows and providing alternative semantics for flow
> + * installation and retrieval.
> *
> * These attributes follow the &struct ovs_header within the Generic Netlink
> * payload for %OVS_FLOW_* commands.
> @@ -486,12 +489,39 @@ enum ovs_flow_attr {
> OVS_FLOW_ATTR_MASK, /* Sequence of OVS_KEY_ATTR_* attributes. */
> OVS_FLOW_ATTR_PROBE, /* Flow operation is a feature probe, error
> * logging should be suppressed. */
> + OVS_FLOW_ATTR_UFID, /* Unique flow identifier. */
> __OVS_FLOW_ATTR_MAX
> };
>
> #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
>
> /**
> + * enum ovs_ufid_attr - Unique identifier types.
> + *
> + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the behaviour of
> + * the current %OVS_FLOW_CMD_* request. Optional for all requests.
> + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
> + */
> +enum ovs_ufid_attr {
> + OVS_UFID_ATTR_UNSPEC,
> + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
> + OVS_UFID_ATTR_ID, /* variable length identifier. */
> + __OVS_UFID_ATTR_MAX
> +};
> +
> +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
> +
> +/**
> + * Omit attributes for notifications.
> + *
> + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the datapath
> + * may omit the corresponding 'ovs_flow_attr' from the response.
> + */
> +#define OVS_UFID_F_OMIT_KEY (1 << 0)
> +#define OVS_UFID_F_OMIT_MASK (1 << 1)
> +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
> +
These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
should be part of enum ovs_flow_attr.
This way we do not need to make UFID nested attr.
> +/**
> * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE action.
> * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample with
> * @OVS_ACTION_ATTR_SAMPLE. A value of 0 samples no packets, a value of
> --
> 1.7.10.4
>
^ permalink raw reply
* Re: [PATCH RFC] net: Pass full skb hash to ndo_rx_flow_steer
From: David Miller @ 2014-11-19 23:56 UTC (permalink / raw)
To: therbert; +Cc: luto, ben, netdev
In-Reply-To: <1416370137-2512-1-git-send-email-therbert@google.com>
From: Tom Herbert <therbert@google.com>
Date: Tue, 18 Nov 2014 20:08:57 -0800
> Currently, for aRFS the index into the flow table is passed to
> ndo_rx_flow_steer as the flow ID of a connection. This is the skb->hash
> & the table mask. It looks like the backend can accept the full
> skb->hash as the flow ID which should reduce the number of collisions
> in the hardware tables.
>
> This patch provides the skb->hash to the driver for flow steering.
> Expiration of HW steered flows was also updated.
>
> With a hash collision in RFS, ndo_rx_flow_steer will continue to be
> called with different CPUs, but now with different flow_ids. If this
> is still too much device interaction, then it might make sense for the
> driver to do its own lookup in its structure to see if a matching
> filter is already installed for a given flow_id, an if it is just
> refresh a timestamp to avoid expiration (based on looking at sfc
> driver).
>
> I don't currently have any HW to test this, if someone could try this
> on hardware with aRFS and provide feedback that would be appreciated.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
This seems legitimate to me.
The only thing a driver can reliably do with the flow_id is
pass it back into rps_may_expire_flow(). Therefore you can
pass it in whatever format you like.
^ permalink raw reply
* Re: [PATCH V2 net] virtio-net: validate features during probe
From: David Miller @ 2014-11-20 0:00 UTC (permalink / raw)
To: jasowang; +Cc: mst, netdev, linux-kernel, virtualization
In-Reply-To: <546C612A.4080807@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 19 Nov 2014 17:21:46 +0800
> More compact, looks good. Thanks
I am assuming there is therefore a V3 of this patch forthcoming.
^ permalink raw reply
* Re: [net-next PATCH v2] ixgbe: Remove IXGBE_FLAG_IN_NETPOLL since it doesn't do anything
From: Jeff Kirsher @ 2014-11-20 0:09 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, donald.c.skidmore
In-Reply-To: <20141119232710.19544.20012.stgit@ahduyck-vm-fedora20>
[-- Attachment #1: Type: text/plain, Size: 838 bytes --]
On Wed, 2014-11-19 at 15:28 -0800, Alexander Duyck wrote:
> This patch removes some dead code from the cleanup path for ixgbe.
>
> Setting and clearing the flag doesn't do anything since all we are
> doing is
> setting the flag, scheduling napi, clearing the flag and then letting
> netpoll do the polling cleanup. As such it doesn't make much sense to
> have
> it there.
>
> This patch also removes one minor white-space error.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>
> v2: Fixed an unused variable warning for adapter in ixgbe_rx_skb
>
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 -
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 18
> ++++--------------
> 2 files changed, 4 insertions(+), 15 deletions(-)
Thanks Alex, I will add your patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 net-next 0/2] bonding: Introduce 4 AD link speed
From: David Miller @ 2014-11-20 0:10 UTC (permalink / raw)
To: Jianhua.Xie; +Cc: netdev, edumazet, j.vosburgh, vfalico, andy
In-Reply-To: <1416386939-24591-1-git-send-email-Jianhua.Xie@freescale.com>
From: Xie Jianhua <Jianhua.Xie@freescale.com>
Date: Wed, 19 Nov 2014 16:48:57 +0800
> The speed field of AD Port Key was based on bitmask, it supported 5
> kinds of link speed at most, as there were only 5 bits in the speed
> field of the AD Port Key. This patches series change the speed type
> (AD_LINK_SPEED_BITMASK) from bitmask to enum type in order to enhance
> speed type from 5 to 32, and then introduce 4 AD link speed to fix
> agg_bandwidth.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: use skb_get_hash_raw instead of skb_get_hash in set_rps_cpu
From: David Miller @ 2014-11-20 0:16 UTC (permalink / raw)
To: _govind; +Cc: netdev, therbert
In-Reply-To: <1416391244-27390-1-git-send-email-_govind@gmx.com>
From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Wed, 19 Nov 2014 15:30:44 +0530
> In set_rps_cpu, we call skb_get_hash to get the hash of skb. The caller
> get_rps_cpu has already determined the hash of skb by calling skb_get_hash.
>
> Since get_rps_cpu is the only caller of set_rps_cpu, we can use skb_get_hash_raw
>
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
Tom, please review. And if you agree with this change, you'll need to
be spinning your flow_id change relative to this I guess since it overlaps.
> ---
> net/core/dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1ab168e..b4ad4d1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3055,7 +3055,7 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
> flow_table = rcu_dereference(rxqueue->rps_flow_table);
> if (!flow_table)
> goto out;
> - flow_id = skb_get_hash(skb) & flow_table->mask;
> + flow_id = skb_get_hash_raw(skb) & flow_table->mask;
> rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
> rxq_index, flow_id);
> if (rc < 0)
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH RFC] net: Pass full skb hash to ndo_rx_flow_steer
From: Andy Lutomirski @ 2014-11-20 0:21 UTC (permalink / raw)
To: Tom Herbert; +Cc: Ben Hutchings, Network Development
In-Reply-To: <1416370137-2512-1-git-send-email-therbert@google.com>
On Tue, Nov 18, 2014 at 8:08 PM, Tom Herbert <therbert@google.com> wrote:
> Currently, for aRFS the index into the flow table is passed to
> ndo_rx_flow_steer as the flow ID of a connection. This is the skb->hash
> & the table mask. It looks like the backend can accept the full
> skb->hash as the flow ID which should reduce the number of collisions
> in the hardware tables.
>
> This patch provides the skb->hash to the driver for flow steering.
> Expiration of HW steered flows was also updated.
>
> With a hash collision in RFS, ndo_rx_flow_steer will continue to be
> called with different CPUs, but now with different flow_ids. If this
> is still too much device interaction, then it might make sense for the
> driver to do its own lookup in its structure to see if a matching
> filter is already installed for a given flow_id, an if it is just
> refresh a timestamp to avoid expiration (based on looking at sfc
> driver).
>
> I don't currently have any HW to test this, if someone could try this
> on hardware with aRFS and provide feedback that would be appreciated.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> net/core/dev.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1ab168e..cb1e06d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3057,7 +3057,8 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
> goto out;
> flow_id = skb_get_hash(skb) & flow_table->mask;
> rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
> - rxq_index, flow_id);
> + rxq_index,
> + skb_get_hash(skb));
Can gcc CSE this? Not that it matters that much.
> if (rc < 0)
> goto out;
> old_rflow = rflow;
> @@ -3195,8 +3196,8 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index,
>
> rcu_read_lock();
> flow_table = rcu_dereference(rxqueue->rps_flow_table);
> - if (flow_table && flow_id <= flow_table->mask) {
> - rflow = &flow_table->flows[flow_id];
> + if (flow_table) {
> + rflow = &flow_table->flows[flow_id & flow_table->mask];
I think this is nicer, but why will it help? If there's a collision
in the low bits of the hash, we'll still think that the flow is
expired.
Is there a real LRU-ish hash table that could be subbed in easily?
There ought to be a data structure that's barely more complicated than
this kind of hash table but that gives much better behavior when the
number of flows is smaller than the table size.
--Andy
> cpu = ACCESS_ONCE(rflow->cpu);
> if (rflow->filter == filter_id && cpu != RPS_NO_CPU &&
> ((int)(per_cpu(softnet_data, cpu).input_queue_head -
> --
> 2.1.0.rc2.206.gedb03e5
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Joe Stringer @ 2014-11-20 0:20 UTC (permalink / raw)
To: Pravin Shelar; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <CALnjE+owCheaKK8=JBmW0whpC1_iP67XfnaGCghi4jSWYaDfkg@mail.gmail.com>
On Wednesday, November 19, 2014 15:34:24 Pravin Shelar wrote:
> On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com>
wrote:
> > @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct
> > sw_flow_actions *acts)
> >
> > /* Called with ovs_mutex or RCU read lock. */
> > static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
> >
> > - struct sk_buff *skb)
> > + struct sk_buff *skb, u32 ufid_flags)
> >
> > {
> >
> > struct nlattr *nla;
> > int err;
> >
> > - /* Fill flow key. */
> > - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> > - if (!nla)
> > - return -EMSGSIZE;
> > -
> > - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key,
> > skb, - false);
> > - if (err)
> > - return err;
> > -
> > - nla_nest_end(skb, nla);
> > + /* Fill flow key. If userspace didn't specify a UFID, then ignore
> > the + * OMIT_KEY flag. */
> > + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
> > + !flow->index_by_ufid) {
>
> I am not sure about this check, userspace needs to send atleast ufid
> or the unmasked key as id for flow. otherwise we shld flag error. Here
> we can serialize flow->key.
> There could be another function which takes care of flow-id
> serialization where we serialize use ufid or unmasked key as flow id.
> Lets group ufid and unmasked key together rather than masked key and
> unmasked key which are not related.
Right, at flow setup time the flow key is always required, but the UFID is
optional. For most other cases, one of the two most be specified. For flow dump,
neither is required from userspace, but OMIT_KEY flag may be raised. That's the
particular case that this logic is trying to catch (dump all flows, including
those that were set up without UFID - in which case the OMIT_KEY flag doesn't
make sense, so treat the flag like a request rather than a command).
Happy to split the key/identifier out from the mask.
> > @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct
> > sw_flow *flow,
> >
> > }
> >
> > /* Called with ovs_mutex or RCU read lock. */
> >
> > +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
> > + struct sk_buff *skb)
> > +{
> > + struct nlattr *start;
> > + const struct sw_flow_id *sfid;
> > +
> > + if (!flow->index_by_ufid)
> > + return 0;
> > +
> > + sfid = &flow->index.ufid;
> > + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
> > + if (start) {
> > + int err;
> > +
> > + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
> > + sfid->ufid);
> > + if (err)
> > + return err;
> > + nla_nest_end(skb, start);
> > + } else
> > + return -EMSGSIZE;
> > +
> > + return 0;
> > +}
> > +
>
> Can you change this function according to comments above?
OK.
> > @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct
> > sw_flow *flow, int dp_ifindex,
> >
> > ovs_header->dp_ifindex = dp_ifindex;
> >
> > - err = ovs_flow_cmd_fill_match(flow, skb);
> > + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
> >
> > if (err)
> >
> > goto error;
> >
> > - err = ovs_flow_cmd_fill_stats(flow, skb);
> > + err = ovs_flow_cmd_fill_ufid(flow, skb);
> >
> > if (err)
> >
> > goto error;
>
> Flow ID should go first in the netlink msg.
OK.
> > @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
> > struct genl_info *info)
> >
> > error = -ENODEV;
> > goto err_unlock_ovs;
> >
> > }
> >
> > +
> >
> > /* Check if this is a duplicate flow */
> >
> > - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
> > + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
>
> Need to check for ufid table to find duplicate ufid entry here.
OK.
> > diff --git a/datapath/flow.h b/datapath/flow.h
> > index 2bbf789..736e0eb 100644
> > --- a/datapath/flow.h
> > +++ b/datapath/flow.h
> > @@ -196,6 +196,13 @@ struct sw_flow_match {
> >
> > struct sw_flow_mask *mask;
> >
> > };
> >
> > +struct sw_flow_id {
> > + struct hlist_node node[2];
> > + u32 hash;
> > + u8 *ufid;
> > + u8 ufid_len;
> > +};
> > +
>
> Lets make ufid array of size 256, we can reject any key greater than
> this. current patch does not support key greater than 256 anyways.
Ok, that sounds reasonable.
> > struct sw_flow_actions {
> >
> > struct rcu_head rcu;
> > u32 actions_len;
> >
> > @@ -212,13 +219,20 @@ struct flow_stats {
> >
> > struct sw_flow {
> >
> > struct rcu_head rcu;
> >
> > - struct hlist_node hash_node[2];
> > - u32 hash;
> > + struct {
> > + struct hlist_node node[2];
> > + u32 hash;
> > + } flow_hash;
>
> This change does not look related to this work.
Right, this is unnecessary leftover from earlier iteration.
> > int stats_last_writer; /* NUMA-node id of the last
> > writer on
> >
> > * 'stats[0]'.
> > */
> >
> > struct sw_flow_key key;
> >
> > - struct sw_flow_key unmasked_key;
> > + bool index_by_ufid; /* Which of the below that
> > userspace + uses to index this
> > flow. */ + union {
> > + struct sw_flow_key unmasked_key;
> > + struct sw_flow_id ufid;
> > + } index;
>
> Rather than storing ufid or unmasked key inside struct flow we can
> keep pointer to these objects, that will save some memory.
OK. Do you still care about the union being there?
> > @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl,
> > struct sw_flow *flow,
> >
> > int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
> >
> > const struct sw_flow_mask *mask)
> >
> > {
> >
> > - struct table_instance *new_ti = NULL;
> > - struct table_instance *ti;
> > + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
> > + struct table_instance *ti, *ufid_ti = NULL;
> >
> > int err;
> >
> > err = flow_mask_insert(table, flow, mask);
> > if (err)
> >
> > return err;
> >
> > - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
> > - flow->mask->range.end);
> > + flow->flow_hash.hash = flow_hash(&flow->key,
> > flow->mask->range.start, +
> > flow->mask->range.end);
> >
> > ti = ovsl_dereference(table->ti);
> > table_instance_insert(ti, flow);
> > table->count++;
> >
> > + if (flow->index_by_ufid) {
> > + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
> > + ufid_ti = ovsl_dereference(table->ufid_ti);
> > + ufid_table_instance_insert(ufid_ti, flow);
> > + table->ufid_count++;
> > + }
> >
> > /* Expand table, if necessary, to make room. */
> > if (table->count > ti->n_buckets)
> >
> > - new_ti = table_instance_expand(ti);
> > + new_ti = flow_table_expand(ti, false);
> >
> > else if (time_after(jiffies, table->last_rehash +
> > REHASH_INTERVAL))
> >
> > - new_ti = table_instance_rehash(ti, ti->n_buckets);
> > + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
> > + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
> > + new_ufid_ti = flow_table_expand(ufid_ti, true);
> >
> > if (new_ti) {
> >
> > rcu_assign_pointer(table->ti, new_ti);
> >
> > - table_instance_destroy(ti, true);
> > + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
> >
> > table->last_rehash = jiffies;
> >
> > }
> >
> > + if (new_ufid_ti) {
> > + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
> > + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
> > + }
> > + return 0;
> > +}
>
> Insert function can be simplified by first updating flow-table and
> then updating ufid table.
Sure.
> > #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
> >
> > /**
> >
> > + * enum ovs_ufid_attr - Unique identifier types.
> > + *
> > + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the
> > behaviour of + * the current %OVS_FLOW_CMD_* request. Optional for all
> > requests. + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
> > + */
> > +enum ovs_ufid_attr {
> > + OVS_UFID_ATTR_UNSPEC,
> > + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
> > + OVS_UFID_ATTR_ID, /* variable length identifier. */
> > + __OVS_UFID_ATTR_MAX
> > +};
> > +
> > +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
> > +
> > +/**
> > + * Omit attributes for notifications.
> > + *
> > + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the
> > datapath + * may omit the corresponding 'ovs_flow_attr' from the
> > response. + */
> > +#define OVS_UFID_F_OMIT_KEY (1 << 0)
> > +#define OVS_UFID_F_OMIT_MASK (1 << 1)
> > +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
> > +
>
> These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
> should be part of enum ovs_flow_attr.
> This way we do not need to make UFID nested attr.
OK, I'll shift it out.
Thanks for the review, I'll work on a fresh revision.
^ permalink raw reply
* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Pravin Shelar @ 2014-11-20 0:39 UTC (permalink / raw)
To: Joe Stringer; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <201411191620.32217.joestringer@nicira.com>
On Wed, Nov 19, 2014 at 4:20 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On Wednesday, November 19, 2014 15:34:24 Pravin Shelar wrote:
>> On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com>
> wrote:
>> > @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct
>> > sw_flow_actions *acts)
>> >
>> > /* Called with ovs_mutex or RCU read lock. */
>> > static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
>> >
>> > - struct sk_buff *skb)
>> > + struct sk_buff *skb, u32 ufid_flags)
>> >
>> > {
>> >
>> > struct nlattr *nla;
>> > int err;
>> >
>> > - /* Fill flow key. */
>> > - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
>> > - if (!nla)
>> > - return -EMSGSIZE;
>> > -
>> > - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key,
>> > skb, - false);
>> > - if (err)
>> > - return err;
>> > -
>> > - nla_nest_end(skb, nla);
>> > + /* Fill flow key. If userspace didn't specify a UFID, then ignore
>> > the + * OMIT_KEY flag. */
>> > + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
>> > + !flow->index_by_ufid) {
>>
>> I am not sure about this check, userspace needs to send atleast ufid
>> or the unmasked key as id for flow. otherwise we shld flag error. Here
>> we can serialize flow->key.
>> There could be another function which takes care of flow-id
>> serialization where we serialize use ufid or unmasked key as flow id.
>> Lets group ufid and unmasked key together rather than masked key and
>> unmasked key which are not related.
>
> Right, at flow setup time the flow key is always required, but the UFID is
> optional. For most other cases, one of the two most be specified. For flow dump,
> neither is required from userspace, but OMIT_KEY flag may be raised. That's the
> particular case that this logic is trying to catch (dump all flows, including
> those that were set up without UFID - in which case the OMIT_KEY flag doesn't
> make sense, so treat the flag like a request rather than a command).
>
How do you handle overlapping flows without the flow id in dump operation?
> Happy to split the key/identifier out from the mask.
>
>> > @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct
>> > sw_flow *flow,
>> >
>> > }
>> >
>> > /* Called with ovs_mutex or RCU read lock. */
>> >
>> > +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
>> > + struct sk_buff *skb)
>> > +{
>> > + struct nlattr *start;
>> > + const struct sw_flow_id *sfid;
>> > +
>> > + if (!flow->index_by_ufid)
>> > + return 0;
>> > +
>> > + sfid = &flow->index.ufid;
>> > + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
>> > + if (start) {
>> > + int err;
>> > +
>> > + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
>> > + sfid->ufid);
>> > + if (err)
>> > + return err;
>> > + nla_nest_end(skb, start);
>> > + } else
>> > + return -EMSGSIZE;
>> > +
>> > + return 0;
>> > +}
>> > +
>>
>> Can you change this function according to comments above?
>
> OK.
>
>> > @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct
>> > sw_flow *flow, int dp_ifindex,
>> >
>> > ovs_header->dp_ifindex = dp_ifindex;
>> >
>> > - err = ovs_flow_cmd_fill_match(flow, skb);
>> > + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
>> >
>> > if (err)
>> >
>> > goto error;
>> >
>> > - err = ovs_flow_cmd_fill_stats(flow, skb);
>> > + err = ovs_flow_cmd_fill_ufid(flow, skb);
>> >
>> > if (err)
>> >
>> > goto error;
>>
>> Flow ID should go first in the netlink msg.
>
> OK.
>
>> > @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
>> > struct genl_info *info)
>> >
>> > error = -ENODEV;
>> > goto err_unlock_ovs;
>> >
>> > }
>> >
>> > +
>> >
>> > /* Check if this is a duplicate flow */
>> >
>> > - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
>> > + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
>>
>> Need to check for ufid table to find duplicate ufid entry here.
>
> OK.
>
>> > diff --git a/datapath/flow.h b/datapath/flow.h
>> > index 2bbf789..736e0eb 100644
>> > --- a/datapath/flow.h
>> > +++ b/datapath/flow.h
>> > @@ -196,6 +196,13 @@ struct sw_flow_match {
>> >
>> > struct sw_flow_mask *mask;
>> >
>> > };
>> >
>> > +struct sw_flow_id {
>> > + struct hlist_node node[2];
>> > + u32 hash;
>> > + u8 *ufid;
>> > + u8 ufid_len;
>> > +};
>> > +
>>
>> Lets make ufid array of size 256, we can reject any key greater than
>> this. current patch does not support key greater than 256 anyways.
>
> Ok, that sounds reasonable.
>
>> > struct sw_flow_actions {
>> >
>> > struct rcu_head rcu;
>> > u32 actions_len;
>> >
>> > @@ -212,13 +219,20 @@ struct flow_stats {
>> >
>> > struct sw_flow {
>> >
>> > struct rcu_head rcu;
>> >
>> > - struct hlist_node hash_node[2];
>> > - u32 hash;
>> > + struct {
>> > + struct hlist_node node[2];
>> > + u32 hash;
>> > + } flow_hash;
>>
>> This change does not look related to this work.
>
> Right, this is unnecessary leftover from earlier iteration.
>
>
>> > int stats_last_writer; /* NUMA-node id of the last
>> > writer on
>> >
>> > * 'stats[0]'.
>> > */
>> >
>> > struct sw_flow_key key;
>> >
>> > - struct sw_flow_key unmasked_key;
>> > + bool index_by_ufid; /* Which of the below that
>> > userspace + uses to index this
>> > flow. */ + union {
>> > + struct sw_flow_key unmasked_key;
>> > + struct sw_flow_id ufid;
>> > + } index;
>>
>> Rather than storing ufid or unmasked key inside struct flow we can
>> keep pointer to these objects, that will save some memory.
>
> OK. Do you still care about the union being there?
>
Lets keep both pointer so that we can get rid of the index_by_fid flag.
>> > @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl,
>> > struct sw_flow *flow,
>> >
>> > int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
>> >
>> > const struct sw_flow_mask *mask)
>> >
>> > {
>> >
>> > - struct table_instance *new_ti = NULL;
>> > - struct table_instance *ti;
>> > + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
>> > + struct table_instance *ti, *ufid_ti = NULL;
>> >
>> > int err;
>> >
>> > err = flow_mask_insert(table, flow, mask);
>> > if (err)
>> >
>> > return err;
>> >
>> > - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
>> > - flow->mask->range.end);
>> > + flow->flow_hash.hash = flow_hash(&flow->key,
>> > flow->mask->range.start, +
>> > flow->mask->range.end);
>> >
>> > ti = ovsl_dereference(table->ti);
>> > table_instance_insert(ti, flow);
>> > table->count++;
>> >
>> > + if (flow->index_by_ufid) {
>> > + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
>> > + ufid_ti = ovsl_dereference(table->ufid_ti);
>> > + ufid_table_instance_insert(ufid_ti, flow);
>> > + table->ufid_count++;
>> > + }
>> >
>> > /* Expand table, if necessary, to make room. */
>> > if (table->count > ti->n_buckets)
>> >
>> > - new_ti = table_instance_expand(ti);
>> > + new_ti = flow_table_expand(ti, false);
>> >
>> > else if (time_after(jiffies, table->last_rehash +
>> > REHASH_INTERVAL))
>> >
>> > - new_ti = table_instance_rehash(ti, ti->n_buckets);
>> > + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
>> > + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
>> > + new_ufid_ti = flow_table_expand(ufid_ti, true);
>> >
>> > if (new_ti) {
>> >
>> > rcu_assign_pointer(table->ti, new_ti);
>> >
>> > - table_instance_destroy(ti, true);
>> > + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
>> >
>> > table->last_rehash = jiffies;
>> >
>> > }
>> >
>> > + if (new_ufid_ti) {
>> > + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
>> > + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
>> > + }
>> > + return 0;
>> > +}
>>
>> Insert function can be simplified by first updating flow-table and
>> then updating ufid table.
>
> Sure.
>
>
>> > #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
>> >
>> > /**
>> >
>> > + * enum ovs_ufid_attr - Unique identifier types.
>> > + *
>> > + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the
>> > behaviour of + * the current %OVS_FLOW_CMD_* request. Optional for all
>> > requests. + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
>> > + */
>> > +enum ovs_ufid_attr {
>> > + OVS_UFID_ATTR_UNSPEC,
>> > + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
>> > + OVS_UFID_ATTR_ID, /* variable length identifier. */
>> > + __OVS_UFID_ATTR_MAX
>> > +};
>> > +
>> > +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
>> > +
>> > +/**
>> > + * Omit attributes for notifications.
>> > + *
>> > + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the
>> > datapath + * may omit the corresponding 'ovs_flow_attr' from the
>> > response. + */
>> > +#define OVS_UFID_F_OMIT_KEY (1 << 0)
>> > +#define OVS_UFID_F_OMIT_MASK (1 << 1)
>> > +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
>> > +
>>
>> These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
>> should be part of enum ovs_flow_attr.
>> This way we do not need to make UFID nested attr.
>
> OK, I'll shift it out.
>
> Thanks for the review, I'll work on a fresh revision.
^ 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