* Re: [PATCH] IPVS: ip_vs_sync.c: local functions should not be exposed globally
From: Julian Anastasov @ 2012-04-26 20:11 UTC (permalink / raw)
To: H Hartley Sweeten; +Cc: Linux Kernel, netdev, David S. Miller, Simon Horman
In-Reply-To: <201204261129.53927.hartleys@visionengravers.com>
Hello,
On Thu, 26 Apr 2012, H Hartley Sweeten wrote:
> Functions not referenced outside of a source file should be marked
> static to prevent it from being exposed globally.
>
> This quiets the sparse warnings:
>
> warning: symbol 'ip_vs_sync_conn_v0' was not declared. Should it be static?
>
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: "David S. Miller" <davem@davemloft.net>
>
> ---
>
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index bf5e538..49a1fe8 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -446,7 +446,7 @@ ip_vs_sync_buff_create_v0(struct netns_ipvs *ipvs)
> * Version 0 , could be switched in by sys_ctl.
> * Add an ip_vs_conn information into the current sync_buff.
> */
> -void ip_vs_sync_conn_v0(struct net *net, struct ip_vs_conn *cp)
> +static void ip_vs_sync_conn_v0(struct net *net, struct ip_vs_conn *cp)
The 3 patches for IPVS look correct but this change
is already not needed after one of our planned changes:
"ipvs: reduce sync rate with time thresholds"
Not sure how we will avoid the collision, may be
Simon will take only the other 2 changes? Or David will
take all changes and we have to rebase?
> {
> struct netns_ipvs *ipvs = net_ipvs(net);
> struct ip_vs_sync_mesg_v0 *m;
> --
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [RFC] allow skb->head to point/alias to first skb frag
From: David Miller @ 2012-04-26 20:12 UTC (permalink / raw)
To: eric.dumazet
Cc: maze, ilpo.jarvinen, rick.jones2, netdev, therbert, ncardwell,
ycheng
In-Reply-To: <1335448252.2775.41.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 26 Apr 2012 15:50:52 +0200
> Here is the raw patch. (tg3 part is dirty and needs some helpers)
>
> I am working on the TCP coalesce part, the splice() helpers (to avoid a
> copy), and split in five different patches for further
> discussion/inclusion.
Ok, I like it.
I was confused into thinking that you were going to put this head page
into skb_shared_info()'s page vector. But you're not.
^ permalink raw reply
* Re: [RFC] allow skb->head to point/alias to first skb frag
From: Eric Dumazet @ 2012-04-26 20:18 UTC (permalink / raw)
To: David Miller
Cc: maze, ilpo.jarvinen, rick.jones2, netdev, therbert, ncardwell,
ycheng
In-Reply-To: <20120426.161221.923108657396544878.davem@davemloft.net>
On Thu, 2012-04-26 at 16:12 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 26 Apr 2012 15:50:52 +0200
>
> > Here is the raw patch. (tg3 part is dirty and needs some helpers)
> >
> > I am working on the TCP coalesce part, the splice() helpers (to avoid a
> > copy), and split in five different patches for further
> > discussion/inclusion.
>
> Ok, I like it.
>
> I was confused into thinking that you were going to put this head page
> into skb_shared_info()'s page vector. But you're not.
>
Well, my first mail was confuse... it was just an idea without prior
thinking. Now the picture is clear.
Thanks
^ permalink raw reply
* Re: [PATCH][RFC] bonding: delete migrated IP addresses from the rlb hash table
From: Jay Vosburgh @ 2012-04-26 20:18 UTC (permalink / raw)
To: Jiri Bohac; +Cc: Andy Gospodarek, netdev
In-Reply-To: <20120420185609.GB16028@midget.suse.cz>
Jiri Bohac <jbohac@suse.cz> wrote:
>I finally got back to this, sorry for such a long delay ...
>Start of this thread is here:
>http://thread.gmane.org/gmane.linux.network/222233
>
>On Wed, Mar 07, 2012 at 05:02:16PM +0100, Jiri Bohac wrote:
>> On Wed, Feb 29, 2012 at 06:12:20PM -0800, Jay Vosburgh wrote:
>> > I've done some initial testing with this, and so far I'm seeing
>> > one problem: every time the local host (with bonding) sends a broadcast
>> > ARP, that ends up flushing the entire RLB table. Well, all entries that
>> > match the IP on the bond that's sending an ARP request, which is just
>> > one address in my testing.
>> >
>> > Anyway, this happens because the switch forwards the broadcast
>> > ARP back around to one of the other bond slaves, and then that
>> > "incoming" ARP bears an ip_src of our already-in-use IP address, and
>> > that matches everything in the table.
>>
>> Good catch! I did not notice this.
>>
>> > Perhaps a check that the ip_src being flushed is not actually in
>> > use locally is warranted?
>>
>> This would not work for the setups where the bonding master is
>> bridget to some other network. I think it would be better to also
>> store the source (server) MAC address in struct client_info and
>> only flush the hash table entries if the MAC address from the
>> incoming APR packet and the source MAC address stored in the hash
>> table differ.
Just to make sure I understand: the additional check you propose
(beyond a check that the IP source address is not locally in use) is for
the purpose of minimizing unnecessary flushes, by insuring that the
address really has moved. Correct?
>> Updated patch follows (compile-tested only) I also
>> fixed the coding style problems you pointed out. As for the
>> forward/reverse naming, it's your call. Should I change it to
>> src/dst?
>
>I now thoroughly tested this updated patch in a real setup. It
>works as intended - it does not purge the entries when receiving the
>looped-back ARP requests. It correctly purges the offending
>entries when an ARP packet arrives with a its SRC IP address
>matching a SRC IP address in the table and having differrent MAC
>address -- exactly the case that would cause the ARP cache
>poisoning if not purged from the table.
>
>Jay, would you please consider applying this? Or do you want me
>to somehow rename the RLB table forward/reverse elements?
I'm going to give this a spin this afternoon, but just skimming
through it, I'm still not that thrilled about the "forward" and
"reverse" terminology applying to "hash by dst" and "hash by src"; why
not just call 'em "dst_next" and "src_next", et al, and cut out the
middle man?
-J
>Re-sending the fixed patch with the original description:
>
>------
>
>
>Bonding in balance-alb mode records information from ARP packets
>passing through the bond in a hash table (rx_hashtbl).
>
>At certain situations (e.g. link change of a slave),
>rlb_update_rx_clients() will send out ARP packets to update ARP
>caches of other hosts on the network to achieve RX load balancing.
>
>The problem is that once an IP address is recorded in the hash
>table, it stays there indefinitely. If this IP address is
>migrated to a different host in the network, bonding still sends
>out ARP packets that poison other systems' ARP caches with
>invalid information.
>
>This patch solves this by looking at all incoming ARP packets,
>and checking if the source IP address is one of the source
>addresses stored in the rx_hashtbl. If it is, the corresponding
>hash table entries are removed. Thus, when an IP address is
>migrated, the first ARP broadcast by its new owner will purge the
>offending entries of rx_hashtbl.
>
>The hash table is hashed by ip_dst. To be able to do the above
>check efficiently (not walking the whole hash table), we need a
>reverse mapping (by ip_src).
>
>I added three new members in struct rlb_client_info:
> rx_hashtbl[x].reverse_first will point to the start of a list of
> entries for which hash(ip_src) == x.
> The list is linked with reverse_next and reverse_prev.
>
>When an incoming ARP packet arrives at rlb_arp_recv()
>rlb_purge_src_ip() can quickly walk only the entries on the
>corresponding lists, i.e. the entries that are likely to contain
>the offending IP address.
>
>Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index f820b26..a938ab6 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -84,6 +84,9 @@ static inline struct arp_pkt *arp_pkt(const struct sk_buff *skb)
>
> /* Forward declaration */
> static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[]);
>+static void rlb_purge_src_ip(struct bonding *bond, struct arp_pkt *arp);
>+static void rlb_delete_table_entry_reverse(struct bonding *bond, u32 index);
>+static void rlb_set_reverse_entry(struct bonding *bond, u32 ip_src_hash, u32 ip_dst_hash);
>
> static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
> {
>@@ -366,6 +369,17 @@ static void rlb_arp_recv(struct sk_buff *skb, struct bonding *bond,
> return;
> }
>
>+ /* We received an ARP from arp->ip_src.
>+ * We might have used this IP address previously (on the bonding host
>+ * itself or on a system that is bridged together with the bond).
>+ * However, if arp->mac_src is different than what is stored in
>+ * rx_hashtbl, some other host is now using the IP and we must prevent
>+ * sending out client updates with this IP address as the source.
>+ * Clean up all hash table entries that have this address as ip_src but
>+ * have a dirrerent mac_src.
>+ */
>+ rlb_purge_src_ip(bond, arp);
>+
> if (arp->op_code == htons(ARPOP_REPLY)) {
> /* update rx hash table for this ARP */
> rlb_update_entry_from_arp(bond, arp);
>@@ -635,6 +649,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> /* update mac address from arp */
> memcpy(client_info->mac_dst, arp->mac_dst, ETH_ALEN);
> }
>+ memcpy(client_info->mac_src, arp->mac_src, ETH_ALEN);
>
> assigned_slave = client_info->slave;
> if (assigned_slave) {
>@@ -657,6 +672,13 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> assigned_slave = rlb_next_rx_slave(bond);
>
> if (assigned_slave) {
>+ if (!(client_info->assigned && client_info->ip_src == arp->ip_src)) {
>+ /* ip_src is going to be updated, fix the reverse hashing */
>+ u32 hash_src = _simple_hash((u8 *)&arp->ip_src, sizeof(arp->ip_src));
>+ rlb_delete_table_entry_reverse(bond, hash_index);
>+ rlb_set_reverse_entry(bond, hash_src, hash_index);
>+ }
>+
> client_info->ip_src = arp->ip_src;
> client_info->ip_dst = arp->ip_dst;
> /* arp->mac_dst is broadcast for arp reqeusts.
>@@ -664,6 +686,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> * upon receiving an arp reply.
> */
> memcpy(client_info->mac_dst, arp->mac_dst, ETH_ALEN);
>+ memcpy(client_info->mac_src, arp->mac_src, ETH_ALEN);
> client_info->slave = assigned_slave;
>
> if (compare_ether_addr_64bits(client_info->mac_dst, mac_bcast)) {
>@@ -769,11 +792,109 @@ static void rlb_rebalance(struct bonding *bond)
> }
>
> /* Caller must hold rx_hashtbl lock */
>-static void rlb_init_table_entry(struct rlb_client_info *entry)
>+static void rlb_init_table_entry_forward(struct rlb_client_info *entry)
> {
>- memset(entry, 0, sizeof(struct rlb_client_info));
> entry->next = RLB_NULL_INDEX;
> entry->prev = RLB_NULL_INDEX;
>+ entry->assigned = 0;
>+ entry->slave = NULL;
>+ entry->tag = 0;
>+}
>+static void rlb_init_table_entry_reverse(struct rlb_client_info *entry)
>+{
>+ entry->reverse_first = RLB_NULL_INDEX;
>+ entry->reverse_prev = RLB_NULL_INDEX;
>+ entry->reverse_next = RLB_NULL_INDEX;
>+}
>+
>+static void rlb_init_table_entry(struct rlb_client_info *entry)
>+{
>+ memset(entry, 0, sizeof(struct rlb_client_info));
>+ rlb_init_table_entry_forward(entry);
>+ rlb_init_table_entry_reverse(entry);
>+}
>+
>+static void rlb_delete_table_entry_forward(struct bonding *bond, u32 index)
>+{
>+ struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+ u32 next_index = bond_info->rx_hashtbl[index].next;
>+ u32 prev_index = bond_info->rx_hashtbl[index].prev;
>+
>+ if (index == bond_info->rx_hashtbl_head)
>+ bond_info->rx_hashtbl_head = next_index;
>+ if (prev_index != RLB_NULL_INDEX)
>+ bond_info->rx_hashtbl[prev_index].next = next_index;
>+ if (next_index != RLB_NULL_INDEX)
>+ bond_info->rx_hashtbl[next_index].prev = prev_index;
>+}
>+
>+static void rlb_delete_table_entry_reverse(struct bonding *bond, u32 index)
>+{
>+ struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+ u32 next_index = bond_info->rx_hashtbl[index].reverse_next;
>+ u32 prev_index = bond_info->rx_hashtbl[index].reverse_prev;
>+
>+ bond_info->rx_hashtbl[index].reverse_next = RLB_NULL_INDEX;
>+ bond_info->rx_hashtbl[index].reverse_prev = RLB_NULL_INDEX;
>+
>+ if (next_index != RLB_NULL_INDEX)
>+ bond_info->rx_hashtbl[next_index].reverse_prev = prev_index;
>+
>+ if (prev_index == RLB_NULL_INDEX)
>+ return;
>+
>+ /* is prev_index pointing to the head of this chain? */
>+ if (bond_info->rx_hashtbl[prev_index].reverse_first == index)
>+ bond_info->rx_hashtbl[prev_index].reverse_first = next_index;
>+ else
>+ bond_info->rx_hashtbl[prev_index].reverse_next = next_index;
>+
>+}
>+
>+static void rlb_delete_table_entry(struct bonding *bond, u32 index)
>+{
>+ struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+ struct rlb_client_info *entry = &(bond_info->rx_hashtbl[index]);
>+
>+ rlb_delete_table_entry_forward(bond, index);
>+ rlb_init_table_entry_forward(entry);
>+
>+ rlb_delete_table_entry_reverse(bond, index);
>+}
>+
>+static void rlb_set_reverse_entry(struct bonding *bond, u32 ip_src_hash, u32 ip_dst_hash)
>+{
>+ struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+ u32 next;
>+
>+ bond_info->rx_hashtbl[ip_dst_hash].reverse_prev = ip_src_hash;
>+ next = bond_info->rx_hashtbl[ip_src_hash].reverse_first;
>+ bond_info->rx_hashtbl[ip_dst_hash].reverse_next = next;
>+ if (next != RLB_NULL_INDEX)
>+ bond_info->rx_hashtbl[next].reverse_prev = ip_dst_hash;
>+ bond_info->rx_hashtbl[ip_src_hash].reverse_first = ip_dst_hash;
>+}
>+
>+/* deletes all rx_hashtbl entries with arp->ip_src if their mac_src does
>+ * not match arp->mac_src */
>+static void rlb_purge_src_ip(struct bonding *bond, struct arp_pkt *arp)
>+{
>+ struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+ u32 ip_src_hash = _simple_hash((u8*)&(arp->ip_src), sizeof(arp->ip_src));
>+ u32 index;
>+
>+ _lock_rx_hashtbl_bh(bond);
>+
>+ index = bond_info->rx_hashtbl[ip_src_hash].reverse_first;
>+ while (index != RLB_NULL_INDEX) {
>+ struct rlb_client_info *entry = &(bond_info->rx_hashtbl[index]);
>+ u32 next_index = entry->reverse_next;
>+ if (entry->ip_src == arp->ip_src &&
>+ compare_ether_addr_64bits(arp->mac_src, entry->mac_src))
>+ rlb_delete_table_entry(bond, index);
>+ index = next_index;
>+ }
>+ _unlock_rx_hashtbl_bh(bond);
> }
>
> static int rlb_initialize(struct bonding *bond)
>@@ -831,21 +952,9 @@ static void rlb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
> while (curr_index != RLB_NULL_INDEX) {
> struct rlb_client_info *curr = &(bond_info->rx_hashtbl[curr_index]);
> u32 next_index = bond_info->rx_hashtbl[curr_index].next;
>- u32 prev_index = bond_info->rx_hashtbl[curr_index].prev;
>-
>- if (curr->tag && (curr->vlan_id == vlan_id)) {
>- if (curr_index == bond_info->rx_hashtbl_head) {
>- bond_info->rx_hashtbl_head = next_index;
>- }
>- if (prev_index != RLB_NULL_INDEX) {
>- bond_info->rx_hashtbl[prev_index].next = next_index;
>- }
>- if (next_index != RLB_NULL_INDEX) {
>- bond_info->rx_hashtbl[next_index].prev = prev_index;
>- }
>
>- rlb_init_table_entry(curr);
>- }
>+ if (curr->tag && (curr->vlan_id == vlan_id))
>+ rlb_delete_table_entry(bond, curr_index);
>
> curr_index = next_index;
> }
>diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
>index 90f140a..8286df52 100644
>--- a/drivers/net/bonding/bond_alb.h
>+++ b/drivers/net/bonding/bond_alb.h
>@@ -100,6 +100,7 @@ struct tlb_client_info {
> struct rlb_client_info {
> __be32 ip_src; /* the server IP address */
> __be32 ip_dst; /* the client IP address */
>+ u8 mac_src[ETH_ALEN]; /* the server MAC address */
> u8 mac_dst[ETH_ALEN]; /* the client MAC address */
> u32 next; /* The next Hash table entry index */
> u32 prev; /* The previous Hash table entry index */
>@@ -108,6 +109,9 @@ struct rlb_client_info {
> struct slave *slave; /* the slave assigned to this client */
> u8 tag; /* flag - need to tag skb */
> unsigned short vlan_id; /* VLAN tag associated with IP address */
>+ u32 reverse_next; /* next entry with same hash(ip_src) */
>+ u32 reverse_prev; /* prev entry with same hash(ip_src) */
>+ u32 reverse_first; /* first entry with hash(ip_src) == this entry's index */
> };
>
> struct tlb_slave_info {
>
>
>
>--
>Jiri Bohac <jbohac@suse.cz>
>SUSE Labs, SUSE CZ
>
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* [PATCH] MAINTAINERS: update sctp maintainer address
From: Vladislav Yasevich @ 2012-04-26 20:27 UTC (permalink / raw)
To: Linux Netdev List
Update my email address.
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
MAINTAINERS | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 962232d..e61dc12 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5885,11 +5885,11 @@ F: Documentation/scsi/st.txt
F: drivers/scsi/st*
SCTP PROTOCOL
-M: Vlad Yasevich <vladislav.yasevich@hp.com>
+M: Vlad Yasevich <vyasevich@gmail.com>
M: Sridhar Samudrala <sri@us.ibm.com>
L: linux-sctp@vger.kernel.org
W: http://lksctp.sourceforge.net
-S: Supported
+S: Maintained
F: Documentation/networking/sctp.txt
F: include/linux/sctp.h
F: include/net/sctp/
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] b44: properly use pr_fmt()
From: Luis R. Rodriguez @ 2012-04-26 20:30 UTC (permalink / raw)
To: David Miller
Cc: zambrano-dY08KVG/lbpWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
arend-dY08KVG/lbpWk0Htik3J/w, joe-6d6DIl74uiNBDgjK7y7TUQ
In-Reply-To: <20120426.160757.1741140873728970494.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Thu, Apr 26, 2012 at 1:07 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: "Luis R. Rodriguez" <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
> Date: Thu, 26 Apr 2012 08:53:30 -0700
>
>> OK I found no pr_fmt warnings at all for make allmodconfig ; make V=1
>> ; on v3.4-rc4. For this and others reason stated then yes, this patch
>> is simply bogus.
>
> This is what I expected, thanks for sorting this out :-)
Thanks for kicking me in the balls.
> We're in the rally!
This is the highlight of my life (8:17) !
Luis
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: update sctp maintainer address
From: David Miller @ 2012-04-26 20:31 UTC (permalink / raw)
To: vladislav.yasevich; +Cc: netdev
In-Reply-To: <4F99AFA6.9030502@hp.com>
From: Vladislav Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 26 Apr 2012 16:27:18 -0400
> Update my email address.
>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
Applied, thanks Vlad.
^ permalink raw reply
* [PATCH net-next 1/3] qlcnic: Register device in FAILED state.
From: Anirban Chakraborty @ 2012-04-26 20:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Dept_NX_Linux_NIC_Driver, Sucheta Chakraborty
From: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
o Without failing probe, register netdevice when device is in FAILED state.
o Device will come up with minimum functionality.
Signed-off-by: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 3 +
.../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 50 ++++++++++++++++---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h | 2 +
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 51 ++++++++++++++++++--
4 files changed, 93 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
index 385a4d5..f419965 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
@@ -1352,6 +1352,8 @@ enum op_codes {
#define QLCNIC_ENABLE_FW_DUMP 0xaddfeed
#define QLCNIC_DISABLE_FW_DUMP 0xbadfeed
#define QLCNIC_FORCE_FW_RESET 0xdeaddead
+#define QLCNIC_SET_QUIESCENT 0xadd00010
+#define QLCNIC_RESET_QUIESCENT 0xadd00020
struct qlcnic_dump_operations {
enum op_codes opcode;
@@ -1559,6 +1561,7 @@ static inline u32 qlcnic_tx_avail(struct qlcnic_host_tx_ring *tx_ring)
}
extern const struct ethtool_ops qlcnic_ethtool_ops;
+extern const struct ethtool_ops qlcnic_ethtool_failed_ops;
struct qlcnic_nic_template {
int (*config_bridged_mode) (struct qlcnic_adapter *, u32);
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index 89ddf7f..f19e11e 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -1132,6 +1132,11 @@ qlcnic_get_dump_flag(struct net_device *netdev, struct ethtool_dump *dump)
struct qlcnic_adapter *adapter = netdev_priv(netdev);
struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
+ if (!fw_dump->tmpl_hdr) {
+ netdev_err(adapter->netdev, "FW Dump not supported\n");
+ return -ENOTSUPP;
+ }
+
if (fw_dump->clr)
dump->len = fw_dump->tmpl_hdr->size + fw_dump->size;
else
@@ -1150,6 +1155,11 @@ qlcnic_get_dump_data(struct net_device *netdev, struct ethtool_dump *dump,
struct qlcnic_adapter *adapter = netdev_priv(netdev);
struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
+ if (!fw_dump->tmpl_hdr) {
+ netdev_err(netdev, "FW Dump not supported\n");
+ return -ENOTSUPP;
+ }
+
if (!fw_dump->clr) {
netdev_info(netdev, "Dump not available\n");
return -EINVAL;
@@ -1180,9 +1190,14 @@ qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
int ret = 0;
struct qlcnic_adapter *adapter = netdev_priv(netdev);
struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
+ u32 state;
switch (val->flag) {
case QLCNIC_FORCE_FW_DUMP_KEY:
+ if (!fw_dump->tmpl_hdr) {
+ netdev_err(netdev, "FW dump not supported\n");
+ return -ENOTSUPP;
+ }
if (!fw_dump->enable) {
netdev_info(netdev, "FW dump not enabled\n");
return ret;
@@ -1196,35 +1211,47 @@ qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
qlcnic_dev_request_reset(adapter);
break;
case QLCNIC_DISABLE_FW_DUMP:
- if (fw_dump->enable) {
+ if (fw_dump->enable && fw_dump->tmpl_hdr) {
netdev_info(netdev, "Disabling FW dump\n");
fw_dump->enable = 0;
}
- break;
+ return ret;
case QLCNIC_ENABLE_FW_DUMP:
- if (!fw_dump->enable && fw_dump->tmpl_hdr) {
+ if (!fw_dump->tmpl_hdr) {
+ netdev_err(netdev, "FW dump not supported\n");
+ return -ENOTSUPP;
+ }
+ if (!fw_dump->enable) {
netdev_info(netdev, "Enabling FW dump\n");
fw_dump->enable = 1;
}
- break;
+ return ret;
case QLCNIC_FORCE_FW_RESET:
netdev_info(netdev, "Forcing a FW reset\n");
qlcnic_dev_request_reset(adapter);
adapter->flags &= ~QLCNIC_FW_RESET_OWNER;
- break;
+ return ret;
+ case QLCNIC_SET_QUIESCENT:
+ case QLCNIC_RESET_QUIESCENT:
+ state = QLCRD32(adapter, QLCNIC_CRB_DEV_STATE);
+ if (state == QLCNIC_DEV_FAILED || (state == QLCNIC_DEV_BADBAD))
+ netdev_info(netdev, "Device in FAILED state\n");
+ return ret;
default:
+ if (!fw_dump->tmpl_hdr) {
+ netdev_err(netdev, "FW dump not supported\n");
+ return -ENOTSUPP;
+ }
if (val->flag > QLCNIC_DUMP_MASK_MAX ||
val->flag < QLCNIC_DUMP_MASK_MIN) {
netdev_info(netdev,
"Invalid dump level: 0x%x\n", val->flag);
- ret = -EINVAL;
- goto out;
+ return -EINVAL;
}
fw_dump->tmpl_hdr->drv_cap_mask = val->flag & 0xff;
netdev_info(netdev, "Driver mask changed to: 0x%x\n",
fw_dump->tmpl_hdr->drv_cap_mask);
}
-out:
return ret;
}
@@ -1258,3 +1285,10 @@ const struct ethtool_ops qlcnic_ethtool_ops = {
.get_dump_data = qlcnic_get_dump_data,
.set_dump = qlcnic_set_dump,
};
+
+const struct ethtool_ops qlcnic_ethtool_failed_ops = {
+ .get_settings = qlcnic_get_settings,
+ .get_drvinfo = qlcnic_get_drvinfo,
+ .set_msglevel = qlcnic_set_msglevel,
+ .get_msglevel = qlcnic_get_msglevel,
+};
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h
index a528193..e6a77fe 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h
@@ -704,6 +704,8 @@ enum {
#define QLCNIC_DEV_FAILED 0x6
#define QLCNIC_DEV_QUISCENT 0x7
+#define QLCNIC_DEV_BADBAD 0xbad0bad0
+
#define QLCNIC_DEV_NPAR_NON_OPER 0 /* NON Operational */
#define QLCNIC_DEV_NPAR_OPER 1 /* NPAR Operational */
#define QLCNIC_DEV_NPAR_OPER_TIMEO 30 /* Operational time out */
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 75c32e8..5c47135 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -338,6 +338,10 @@ static const struct net_device_ops qlcnic_netdev_ops = {
#endif
};
+static const struct net_device_ops qlcnic_netdev_failed_ops = {
+ .ndo_open = qlcnic_open,
+};
+
static struct qlcnic_nic_template qlcnic_ops = {
.config_bridged_mode = qlcnic_config_bridged_mode,
.config_led = qlcnic_config_led,
@@ -1623,8 +1627,9 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
err = adapter->nic_ops->start_firmware(adapter);
if (err) {
- dev_err(&pdev->dev, "Loading fw failed.Please Reboot\n");
- goto err_out_decr_ref;
+ dev_err(&pdev->dev, "Loading fw failed. Please Reboot\n"
+ "\t\tIf reboot doesn't help, try flashing the card\n");
+ goto err_out_maintenance_mode;
}
if (qlcnic_read_mac_addr(adapter))
@@ -1695,6 +1700,18 @@ err_out_disable_pdev:
pci_set_drvdata(pdev, NULL);
pci_disable_device(pdev);
return err;
+
+err_out_maintenance_mode:
+ netdev->netdev_ops = &qlcnic_netdev_failed_ops;
+ SET_ETHTOOL_OPS(netdev, &qlcnic_ethtool_failed_ops);
+ err = register_netdev(netdev);
+ if (err) {
+ dev_err(&pdev->dev, "failed to register net device\n");
+ goto err_out_decr_ref;
+ }
+ pci_set_drvdata(pdev, adapter);
+ qlcnic_create_diag_entries(adapter);
+ return 0;
}
static void __devexit qlcnic_remove(struct pci_dev *pdev)
@@ -1831,8 +1848,14 @@ done:
static int qlcnic_open(struct net_device *netdev)
{
struct qlcnic_adapter *adapter = netdev_priv(netdev);
+ u32 state = QLCRD32(adapter, QLCNIC_CRB_DEV_STATE);
int err;
+ if (state == QLCNIC_DEV_FAILED || (state == QLCNIC_DEV_BADBAD)) {
+ netdev_err(netdev, "Device in FAILED state\n");
+ return -EIO;
+ }
+
netif_carrier_off(netdev);
err = qlcnic_attach(adapter);
@@ -3018,6 +3041,12 @@ qlcnic_dev_request_reset(struct qlcnic_adapter *adapter)
return;
state = QLCRD32(adapter, QLCNIC_CRB_DEV_STATE);
+ if (state == QLCNIC_DEV_FAILED || (state == QLCNIC_DEV_BADBAD)) {
+ netdev_err(adapter->netdev,
+ "Device is in FAILED state, Please Reboot\n");
+ qlcnic_api_unlock(adapter);
+ return;
+ }
if (state == QLCNIC_DEV_READY) {
QLCWR32(adapter, QLCNIC_CRB_DEV_STATE, QLCNIC_DEV_NEED_RESET);
@@ -3061,6 +3090,9 @@ qlcnic_cancel_fw_work(struct qlcnic_adapter *adapter)
while (test_and_set_bit(__QLCNIC_RESETTING, &adapter->state))
msleep(10);
+ if (!adapter->fw_work.work.func)
+ return;
+
cancel_delayed_work_sync(&adapter->fw_work);
}
@@ -4280,6 +4312,7 @@ static void
qlcnic_create_diag_entries(struct qlcnic_adapter *adapter)
{
struct device *dev = &adapter->pdev->dev;
+ u32 state = QLCRD32(adapter, QLCNIC_CRB_DEV_STATE);
if (device_create_bin_file(dev, &bin_attr_port_stats))
dev_info(dev, "failed to create port stats sysfs entry");
@@ -4288,14 +4321,19 @@ qlcnic_create_diag_entries(struct qlcnic_adapter *adapter)
return;
if (device_create_file(dev, &dev_attr_diag_mode))
dev_info(dev, "failed to create diag_mode sysfs entry\n");
- if (device_create_file(dev, &dev_attr_beacon))
- dev_info(dev, "failed to create beacon sysfs entry");
if (device_create_bin_file(dev, &bin_attr_crb))
dev_info(dev, "failed to create crb sysfs entry\n");
if (device_create_bin_file(dev, &bin_attr_mem))
dev_info(dev, "failed to create mem sysfs entry\n");
+
+ if (state == QLCNIC_DEV_FAILED || (state == QLCNIC_DEV_BADBAD))
+ return;
+
if (device_create_bin_file(dev, &bin_attr_pci_config))
dev_info(dev, "failed to create pci config sysfs entry");
+ if (device_create_file(dev, &dev_attr_beacon))
+ dev_info(dev, "failed to create beacon sysfs entry");
+
if (!(adapter->flags & QLCNIC_ESWITCH_ENABLED))
return;
if (device_create_bin_file(dev, &bin_attr_esw_config))
@@ -4314,16 +4352,19 @@ static void
qlcnic_remove_diag_entries(struct qlcnic_adapter *adapter)
{
struct device *dev = &adapter->pdev->dev;
+ u32 state = QLCRD32(adapter, QLCNIC_CRB_DEV_STATE);
device_remove_bin_file(dev, &bin_attr_port_stats);
if (adapter->op_mode == QLCNIC_NON_PRIV_FUNC)
return;
device_remove_file(dev, &dev_attr_diag_mode);
- device_remove_file(dev, &dev_attr_beacon);
device_remove_bin_file(dev, &bin_attr_crb);
device_remove_bin_file(dev, &bin_attr_mem);
+ if (state == QLCNIC_DEV_FAILED || (state == QLCNIC_DEV_BADBAD))
+ return;
device_remove_bin_file(dev, &bin_attr_pci_config);
+ device_remove_file(dev, &dev_attr_beacon);
if (!(adapter->flags & QLCNIC_ESWITCH_ENABLED))
return;
device_remove_bin_file(dev, &bin_attr_esw_config);
--
1.7.4.1
^ permalink raw reply related
* [PATCH net-next 0/3] qlcnic: Bug fixes
From: Anirban Chakraborty @ 2012-04-26 20:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Dept_NX_Linux_NIC_Driver, Anirban Chakraborty
In-Reply-To: <1335472292-24335-1-git-send-email-anirban.chakraborty@qlogic.com>
Please apply to net-next.
Thanks,
Anirban
^ permalink raw reply
* [PATCH net-next 3/3] qlcnic: Allow a predefined set of capture masks for FW dump
From: Anirban Chakraborty @ 2012-04-26 20:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Dept_NX_Linux_NIC_Driver, Manish Chopra
In-Reply-To: <1335472292-24335-1-git-send-email-anirban.chakraborty@qlogic.com>
From: Manish Chopra <manish.chopra@qlogic.com>
o 0x3, 0x7, 0xF, 0x1F, 0x3F, 0x7F and 0xFF are the allowed capture masks.
o Updated driver version to 5.0.28
Signed-off-by: Manish chopra <manish.chopra@qlogic.com>
Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 4 +-
.../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 34 ++++++++++---------
drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h | 4 ++
3 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
index a7e2f74..8680a5d 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
@@ -36,8 +36,8 @@
#define _QLCNIC_LINUX_MAJOR 5
#define _QLCNIC_LINUX_MINOR 0
-#define _QLCNIC_LINUX_SUBVERSION 27
-#define QLCNIC_LINUX_VERSIONID "5.0.27"
+#define _QLCNIC_LINUX_SUBVERSION 28
+#define QLCNIC_LINUX_VERSIONID "5.0.28"
#define QLCNIC_DRV_IDC_VER 0x01
#define QLCNIC_DRIVER_VERSION ((_QLCNIC_LINUX_MAJOR << 16) |\
(_QLCNIC_LINUX_MINOR << 8) | (_QLCNIC_LINUX_SUBVERSION))
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index 5f2ad81..735423f 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -1278,7 +1278,7 @@ qlcnic_get_dump_data(struct net_device *netdev, struct ethtool_dump *dump,
static int
qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
{
- int ret = 0;
+ int i;
struct qlcnic_adapter *adapter = netdev_priv(netdev);
struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
u32 state;
@@ -1291,12 +1291,12 @@ qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
}
if (!fw_dump->enable) {
netdev_info(netdev, "FW dump not enabled\n");
- return ret;
+ return 0;
}
if (fw_dump->clr) {
netdev_info(netdev,
"Previous dump not cleared, not forcing dump\n");
- return ret;
+ return 0;
}
netdev_info(netdev, "Forcing a FW dump\n");
qlcnic_dev_request_reset(adapter);
@@ -1306,7 +1306,7 @@ qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
netdev_info(netdev, "Disabling FW dump\n");
fw_dump->enable = 0;
}
- return ret;
+ return 0;
case QLCNIC_ENABLE_FW_DUMP:
if (!fw_dump->tmpl_hdr) {
netdev_err(netdev, "FW dump not supported\n");
@@ -1316,34 +1316,36 @@ qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
netdev_info(netdev, "Enabling FW dump\n");
fw_dump->enable = 1;
}
- return ret;
+ return 0;
case QLCNIC_FORCE_FW_RESET:
netdev_info(netdev, "Forcing a FW reset\n");
qlcnic_dev_request_reset(adapter);
adapter->flags &= ~QLCNIC_FW_RESET_OWNER;
- return ret;
+ return 0;
case QLCNIC_SET_QUIESCENT:
case QLCNIC_RESET_QUIESCENT:
state = QLCRD32(adapter, QLCNIC_CRB_DEV_STATE);
if (state == QLCNIC_DEV_FAILED || (state == QLCNIC_DEV_BADBAD))
netdev_info(netdev, "Device in FAILED state\n");
- return ret;
+ return 0;
default:
if (!fw_dump->tmpl_hdr) {
netdev_err(netdev, "FW dump not supported\n");
return -ENOTSUPP;
}
- if (val->flag > QLCNIC_DUMP_MASK_MAX ||
- val->flag < QLCNIC_DUMP_MASK_MIN) {
- netdev_info(netdev,
- "Invalid dump level: 0x%x\n", val->flag);
- return -EINVAL;
+ for (i = 0; i < ARRAY_SIZE(FW_DUMP_LEVELS); i++) {
+ if (val->flag == FW_DUMP_LEVELS[i]) {
+ fw_dump->tmpl_hdr->drv_cap_mask =
+ val->flag;
+ netdev_info(netdev, "Driver mask changed to: 0x%x\n",
+ fw_dump->tmpl_hdr->drv_cap_mask);
+ return 0;
+ }
}
- fw_dump->tmpl_hdr->drv_cap_mask = val->flag & 0xff;
- netdev_info(netdev, "Driver mask changed to: 0x%x\n",
- fw_dump->tmpl_hdr->drv_cap_mask);
+ netdev_info(netdev, "Invalid dump level: 0x%x\n", val->flag);
+ return -EINVAL;
}
- return ret;
+ return 0;
}
const struct ethtool_ops qlcnic_ethtool_ops = {
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h
index e6a77fe..6ced319 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h
@@ -778,6 +778,10 @@ struct qlcnic_legacy_intr_set {
#define FLASH_ROM_WINDOW 0x42110030
#define FLASH_ROM_DATA 0x42150000
+
+static const u32 FW_DUMP_LEVELS[] = {
+ 0x3, 0x7, 0xf, 0x1f, 0x3f, 0x7f, 0xff };
+
static const u32 MIU_TEST_READ_DATA[] = {
0x410000A8, 0x410000AC, 0x410000B8, 0x410000BC, };
--
1.7.4.1
^ permalink raw reply related
* [PATCH net-next 2/3] qlcnic: Adding mac statistics to ethtool.
From: Anirban Chakraborty @ 2012-04-26 20:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Dept_NX_Linux_NIC_Driver, Jitendra Kalsaria
In-Reply-To: <1335472292-24335-1-git-send-email-anirban.chakraborty@qlogic.com>
From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 56 ++++++++-
drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 73 ++++++++++-
.../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 127 +++++++++++++++++---
3 files changed, 226 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
index f419965..a7e2f74 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
@@ -607,6 +607,7 @@ struct qlcnic_recv_context {
#define QLCNIC_CDRP_CMD_CONFIG_PORT 0x0000002E
#define QLCNIC_CDRP_CMD_TEMP_SIZE 0x0000002f
#define QLCNIC_CDRP_CMD_GET_TEMP_HDR 0x00000030
+#define QLCNIC_CDRP_CMD_GET_MAC_STATS 0x00000037
#define QLCNIC_RCODE_SUCCESS 0
#define QLCNIC_RCODE_NOT_SUPPORTED 9
@@ -1180,18 +1181,62 @@ struct qlcnic_esw_func_cfg {
#define QLCNIC_STATS_ESWITCH 2
#define QLCNIC_QUERY_RX_COUNTER 0
#define QLCNIC_QUERY_TX_COUNTER 1
-#define QLCNIC_ESW_STATS_NOT_AVAIL 0xffffffffffffffffULL
+#define QLCNIC_STATS_NOT_AVAIL 0xffffffffffffffffULL
+#define QLCNIC_FILL_STATS(VAL1) \
+ (((VAL1) == QLCNIC_STATS_NOT_AVAIL) ? 0 : VAL1)
+#define QLCNIC_MAC_STATS 1
+#define QLCNIC_ESW_STATS 2
#define QLCNIC_ADD_ESW_STATS(VAL1, VAL2)\
do { \
- if (((VAL1) == QLCNIC_ESW_STATS_NOT_AVAIL) && \
- ((VAL2) != QLCNIC_ESW_STATS_NOT_AVAIL)) \
+ if (((VAL1) == QLCNIC_STATS_NOT_AVAIL) && \
+ ((VAL2) != QLCNIC_STATS_NOT_AVAIL)) \
(VAL1) = (VAL2); \
- else if (((VAL1) != QLCNIC_ESW_STATS_NOT_AVAIL) && \
- ((VAL2) != QLCNIC_ESW_STATS_NOT_AVAIL)) \
+ else if (((VAL1) != QLCNIC_STATS_NOT_AVAIL) && \
+ ((VAL2) != QLCNIC_STATS_NOT_AVAIL)) \
(VAL1) += (VAL2); \
} while (0)
+struct qlcnic_mac_statistics{
+ __le64 mac_tx_frames;
+ __le64 mac_tx_bytes;
+ __le64 mac_tx_mcast_pkts;
+ __le64 mac_tx_bcast_pkts;
+ __le64 mac_tx_pause_cnt;
+ __le64 mac_tx_ctrl_pkt;
+ __le64 mac_tx_lt_64b_pkts;
+ __le64 mac_tx_lt_127b_pkts;
+ __le64 mac_tx_lt_255b_pkts;
+ __le64 mac_tx_lt_511b_pkts;
+ __le64 mac_tx_lt_1023b_pkts;
+ __le64 mac_tx_lt_1518b_pkts;
+ __le64 mac_tx_gt_1518b_pkts;
+ __le64 rsvd1[3];
+
+ __le64 mac_rx_frames;
+ __le64 mac_rx_bytes;
+ __le64 mac_rx_mcast_pkts;
+ __le64 mac_rx_bcast_pkts;
+ __le64 mac_rx_pause_cnt;
+ __le64 mac_rx_ctrl_pkt;
+ __le64 mac_rx_lt_64b_pkts;
+ __le64 mac_rx_lt_127b_pkts;
+ __le64 mac_rx_lt_255b_pkts;
+ __le64 mac_rx_lt_511b_pkts;
+ __le64 mac_rx_lt_1023b_pkts;
+ __le64 mac_rx_lt_1518b_pkts;
+ __le64 mac_rx_gt_1518b_pkts;
+ __le64 rsvd2[3];
+
+ __le64 mac_rx_length_error;
+ __le64 mac_rx_length_small;
+ __le64 mac_rx_length_large;
+ __le64 mac_rx_jabber;
+ __le64 mac_rx_dropped;
+ __le64 mac_rx_crc_error;
+ __le64 mac_align_error;
+} __packed;
+
struct __qlcnic_esw_statistics {
__le16 context_id;
__le16 version;
@@ -1512,6 +1557,7 @@ int qlcnic_get_port_stats(struct qlcnic_adapter *, const u8, const u8,
int qlcnic_get_eswitch_stats(struct qlcnic_adapter *, const u8, u8,
struct __qlcnic_esw_statistics *);
int qlcnic_clear_esw_stats(struct qlcnic_adapter *adapter, u8, u8, u8);
+int qlcnic_get_mac_stats(struct qlcnic_adapter *, struct qlcnic_mac_statistics *);
extern int qlcnic_config_tso;
/*
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
index 569a837..8db8524 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
@@ -905,6 +905,65 @@ int qlcnic_get_port_stats(struct qlcnic_adapter *adapter, const u8 func,
return err;
}
+/* This routine will retrieve the MAC statistics from firmware */
+int qlcnic_get_mac_stats(struct qlcnic_adapter *adapter,
+ struct qlcnic_mac_statistics *mac_stats)
+{
+ struct qlcnic_mac_statistics *stats;
+ struct qlcnic_cmd_args cmd;
+ size_t stats_size = sizeof(struct qlcnic_mac_statistics);
+ dma_addr_t stats_dma_t;
+ void *stats_addr;
+ int err;
+
+ stats_addr = dma_alloc_coherent(&adapter->pdev->dev, stats_size,
+ &stats_dma_t, GFP_KERNEL);
+ if (!stats_addr) {
+ dev_err(&adapter->pdev->dev,
+ "%s: Unable to allocate memory.\n", __func__);
+ return -ENOMEM;
+ }
+ memset(stats_addr, 0, stats_size);
+ memset(&cmd, 0, sizeof(cmd));
+ cmd.req.cmd = QLCNIC_CDRP_CMD_GET_MAC_STATS;
+ cmd.req.arg1 = stats_size << 16;
+ cmd.req.arg2 = MSD(stats_dma_t);
+ cmd.req.arg3 = LSD(stats_dma_t);
+
+ qlcnic_issue_cmd(adapter, &cmd);
+ err = cmd.rsp.cmd;
+
+ if (!err) {
+ stats = stats_addr;
+ mac_stats->mac_tx_frames = le64_to_cpu(stats->mac_tx_frames);
+ mac_stats->mac_tx_bytes = le64_to_cpu(stats->mac_tx_bytes);
+ mac_stats->mac_tx_mcast_pkts =
+ le64_to_cpu(stats->mac_tx_mcast_pkts);
+ mac_stats->mac_tx_bcast_pkts =
+ le64_to_cpu(stats->mac_tx_bcast_pkts);
+ mac_stats->mac_rx_frames = le64_to_cpu(stats->mac_rx_frames);
+ mac_stats->mac_rx_bytes = le64_to_cpu(stats->mac_rx_bytes);
+ mac_stats->mac_rx_mcast_pkts =
+ le64_to_cpu(stats->mac_rx_mcast_pkts);
+ mac_stats->mac_rx_length_error =
+ le64_to_cpu(stats->mac_rx_length_error);
+ mac_stats->mac_rx_length_small =
+ le64_to_cpu(stats->mac_rx_length_small);
+ mac_stats->mac_rx_length_large =
+ le64_to_cpu(stats->mac_rx_length_large);
+ mac_stats->mac_rx_jabber = le64_to_cpu(stats->mac_rx_jabber);
+ mac_stats->mac_rx_dropped = le64_to_cpu(stats->mac_rx_dropped);
+ mac_stats->mac_rx_crc_error = le64_to_cpu(stats->mac_rx_crc_error);
+ } else {
+ dev_info(&adapter->pdev->dev,
+ "%s: Get mac stats failed =%d.\n", __func__, err);
+ }
+
+ dma_free_coherent(&adapter->pdev->dev, stats_size, stats_addr,
+ stats_dma_t);
+ return err;
+}
+
int qlcnic_get_eswitch_stats(struct qlcnic_adapter *adapter, const u8 eswitch,
const u8 rx_tx, struct __qlcnic_esw_statistics *esw_stats) {
@@ -920,13 +979,13 @@ int qlcnic_get_eswitch_stats(struct qlcnic_adapter *adapter, const u8 eswitch,
return -EIO;
memset(esw_stats, 0, sizeof(u64));
- esw_stats->unicast_frames = QLCNIC_ESW_STATS_NOT_AVAIL;
- esw_stats->multicast_frames = QLCNIC_ESW_STATS_NOT_AVAIL;
- esw_stats->broadcast_frames = QLCNIC_ESW_STATS_NOT_AVAIL;
- esw_stats->dropped_frames = QLCNIC_ESW_STATS_NOT_AVAIL;
- esw_stats->errors = QLCNIC_ESW_STATS_NOT_AVAIL;
- esw_stats->local_frames = QLCNIC_ESW_STATS_NOT_AVAIL;
- esw_stats->numbytes = QLCNIC_ESW_STATS_NOT_AVAIL;
+ esw_stats->unicast_frames = QLCNIC_STATS_NOT_AVAIL;
+ esw_stats->multicast_frames = QLCNIC_STATS_NOT_AVAIL;
+ esw_stats->broadcast_frames = QLCNIC_STATS_NOT_AVAIL;
+ esw_stats->dropped_frames = QLCNIC_STATS_NOT_AVAIL;
+ esw_stats->errors = QLCNIC_STATS_NOT_AVAIL;
+ esw_stats->local_frames = QLCNIC_STATS_NOT_AVAIL;
+ esw_stats->numbytes = QLCNIC_STATS_NOT_AVAIL;
esw_stats->context_id = eswitch;
for (i = 0; i < QLCNIC_MAX_PCI_FUNC; i++) {
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index f19e11e..5f2ad81 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -78,8 +78,46 @@ static const char qlcnic_device_gstrings_stats[][ETH_GSTRING_LEN] = {
"tx numbytes",
};
-#define QLCNIC_STATS_LEN ARRAY_SIZE(qlcnic_gstrings_stats)
+static const char qlcnic_mac_stats_strings [][ETH_GSTRING_LEN] = {
+ "mac_tx_frames",
+ "mac_tx_bytes",
+ "mac_tx_mcast_pkts",
+ "mac_tx_bcast_pkts",
+ "mac_tx_pause_cnt",
+ "mac_tx_ctrl_pkt",
+ "mac_tx_lt_64b_pkts",
+ "mac_tx_lt_127b_pkts",
+ "mac_tx_lt_255b_pkts",
+ "mac_tx_lt_511b_pkts",
+ "mac_tx_lt_1023b_pkts",
+ "mac_tx_lt_1518b_pkts",
+ "mac_tx_gt_1518b_pkts",
+ "mac_rx_frames",
+ "mac_rx_bytes",
+ "mac_rx_mcast_pkts",
+ "mac_rx_bcast_pkts",
+ "mac_rx_pause_cnt",
+ "mac_rx_ctrl_pkt",
+ "mac_rx_lt_64b_pkts",
+ "mac_rx_lt_127b_pkts",
+ "mac_rx_lt_255b_pkts",
+ "mac_rx_lt_511b_pkts",
+ "mac_rx_lt_1023b_pkts",
+ "mac_rx_lt_1518b_pkts",
+ "mac_rx_gt_1518b_pkts",
+ "mac_rx_length_error",
+ "mac_rx_length_small",
+ "mac_rx_length_large",
+ "mac_rx_jabber",
+ "mac_rx_dropped",
+ "mac_rx_crc_error",
+ "mac_align_error",
+};
+
+#define QLCNIC_STATS_LEN ARRAY_SIZE(qlcnic_gstrings_stats)
+#define QLCNIC_MAC_STATS_LEN ARRAY_SIZE(qlcnic_mac_stats_strings)
#define QLCNIC_DEVICE_STATS_LEN ARRAY_SIZE(qlcnic_device_gstrings_stats)
+#define QLCNIC_TOTAL_STATS_LEN QLCNIC_STATS_LEN + QLCNIC_MAC_STATS_LEN
static const char qlcnic_gstrings_test[][ETH_GSTRING_LEN] = {
"Register_Test_on_offline",
@@ -644,8 +682,8 @@ static int qlcnic_get_sset_count(struct net_device *dev, int sset)
return QLCNIC_TEST_LEN;
case ETH_SS_STATS:
if (adapter->flags & QLCNIC_ESWITCH_ENABLED)
- return QLCNIC_STATS_LEN + QLCNIC_DEVICE_STATS_LEN;
- return QLCNIC_STATS_LEN;
+ return QLCNIC_TOTAL_STATS_LEN + QLCNIC_DEVICE_STATS_LEN;
+ return QLCNIC_TOTAL_STATS_LEN;
default:
return -EOPNOTSUPP;
}
@@ -851,7 +889,7 @@ static void
qlcnic_get_strings(struct net_device *dev, u32 stringset, u8 * data)
{
struct qlcnic_adapter *adapter = netdev_priv(dev);
- int index, i;
+ int index, i, j;
switch (stringset) {
case ETH_SS_TEST:
@@ -864,6 +902,11 @@ qlcnic_get_strings(struct net_device *dev, u32 stringset, u8 * data)
qlcnic_gstrings_stats[index].stat_string,
ETH_GSTRING_LEN);
}
+ for (j = 0; j < QLCNIC_MAC_STATS_LEN; index++, j++) {
+ memcpy(data + index * ETH_GSTRING_LEN,
+ qlcnic_mac_stats_strings[j],
+ ETH_GSTRING_LEN);
+ }
if (!(adapter->flags & QLCNIC_ESWITCH_ENABLED))
return;
for (i = 0; i < QLCNIC_DEVICE_STATS_LEN; index++, i++) {
@@ -874,22 +917,64 @@ qlcnic_get_strings(struct net_device *dev, u32 stringset, u8 * data)
}
}
-#define QLCNIC_FILL_ESWITCH_STATS(VAL1) \
- (((VAL1) == QLCNIC_ESW_STATS_NOT_AVAIL) ? 0 : VAL1)
-
static void
-qlcnic_fill_device_stats(int *index, u64 *data,
- struct __qlcnic_esw_statistics *stats)
+qlcnic_fill_stats(int *index, u64 *data, void *stats, int type)
{
int ind = *index;
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->unicast_frames);
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->multicast_frames);
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->broadcast_frames);
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->dropped_frames);
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->errors);
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->local_frames);
- data[ind++] = QLCNIC_FILL_ESWITCH_STATS(stats->numbytes);
+ if (type == QLCNIC_MAC_STATS) {
+ struct qlcnic_mac_statistics *mac_stats =
+ (struct qlcnic_mac_statistics *)stats;
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_frames);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_bytes);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_mcast_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_bcast_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_pause_cnt);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_ctrl_pkt);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_lt_64b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_lt_127b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_lt_255b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_tx_lt_511b_pkts);
+ data[ind++] =
+ QLCNIC_FILL_STATS(mac_stats->mac_tx_lt_1023b_pkts);
+ data[ind++] =
+ QLCNIC_FILL_STATS(mac_stats->mac_tx_lt_1518b_pkts);
+ data[ind++] =
+ QLCNIC_FILL_STATS(mac_stats->mac_tx_gt_1518b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_frames);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_bytes);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_mcast_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_bcast_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_pause_cnt);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_ctrl_pkt);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_lt_64b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_lt_127b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_lt_255b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_lt_511b_pkts);
+ data[ind++] =
+ QLCNIC_FILL_STATS(mac_stats->mac_rx_lt_1023b_pkts);
+ data[ind++] =
+ QLCNIC_FILL_STATS(mac_stats->mac_rx_lt_1518b_pkts);
+ data[ind++] =
+ QLCNIC_FILL_STATS(mac_stats->mac_rx_gt_1518b_pkts);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_length_error);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_length_small);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_length_large);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_jabber);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_dropped);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_rx_crc_error);
+ data[ind++] = QLCNIC_FILL_STATS(mac_stats->mac_align_error);
+ } else if (type == QLCNIC_ESW_STATS) {
+ struct __qlcnic_esw_statistics *esw_stats =
+ (struct __qlcnic_esw_statistics *)stats;
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->unicast_frames);
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->multicast_frames);
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->broadcast_frames);
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->dropped_frames);
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->errors);
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->local_frames);
+ data[ind++] = QLCNIC_FILL_STATS(esw_stats->numbytes);
+ }
*index = ind;
}
@@ -900,6 +985,7 @@ qlcnic_get_ethtool_stats(struct net_device *dev,
{
struct qlcnic_adapter *adapter = netdev_priv(dev);
struct qlcnic_esw_statistics port_stats;
+ struct qlcnic_mac_statistics mac_stats;
int index, ret;
for (index = 0; index < QLCNIC_STATS_LEN; index++) {
@@ -911,6 +997,11 @@ qlcnic_get_ethtool_stats(struct net_device *dev,
sizeof(u64)) ? *(u64 *)p:(*(u32 *)p);
}
+ /* Retrieve MAC statistics from firmware */
+ memset(&mac_stats, 0, sizeof(struct qlcnic_mac_statistics));
+ qlcnic_get_mac_stats(adapter, &mac_stats);
+ qlcnic_fill_stats(&index, data, &mac_stats, QLCNIC_MAC_STATS);
+
if (!(adapter->flags & QLCNIC_ESWITCH_ENABLED))
return;
@@ -920,14 +1011,14 @@ qlcnic_get_ethtool_stats(struct net_device *dev,
if (ret)
return;
- qlcnic_fill_device_stats(&index, data, &port_stats.rx);
+ qlcnic_fill_stats(&index, data, &port_stats.rx, QLCNIC_ESW_STATS);
ret = qlcnic_get_port_stats(adapter, adapter->ahw->pci_func,
QLCNIC_QUERY_TX_COUNTER, &port_stats.tx);
if (ret)
return;
- qlcnic_fill_device_stats(&index, data, &port_stats.tx);
+ qlcnic_fill_stats(&index, data, &port_stats.tx, QLCNIC_ESW_STATS);
}
static int qlcnic_set_led(struct net_device *dev,
--
1.7.4.1
^ permalink raw reply related
* Re: [Xen-devel] [PATCH 06/10] net: add support for per-paged-fragment destructors
From: Konrad Rzeszutek Wilk @ 2012-04-26 20:44 UTC (permalink / raw)
To: Ian Campbell
Cc: netdev, Wei Liu, Eric Dumazet, Michael S. Tsirkin, xen-devel,
Michał Mirosław, David Miller
In-Reply-To: <1334067984-7706-6-git-send-email-ian.campbell@citrix.com>
On Tue, Apr 10, 2012 at 03:26:20PM +0100, Ian Campbell wrote:
> Entities which care about the complete lifecycle of pages which they inject
> into the network stack via an skb paged fragment can choose to set this
> destructor in order to receive a callback when the stack is really finished
> with a page (including all clones, retransmits, pull-ups etc etc).
>
> This destructor will always be propagated alongside the struct page when
> copying skb_frag_t->page. This is the reason I chose to embed the destructor in
> a "struct { } page" within the skb_frag_t, rather than as a separate field,
> since it allows existing code which propagates ->frags[N].page to Just
> Work(tm).
>
> When the destructor is present the page reference counting is done slightly
> differently. No references are held by the network stack on the struct page (it
> is up to the caller to manage this as necessary) instead the network stack will
> track references via the count embedded in the destructor structure. When this
> reference count reaches zero then the destructor will be called and the caller
> can take the necesary steps to release the page (i.e. release the struct page
> reference itself).
>
> The intention is that callers can use this callback to delay completion to
> _their_ callers until the network stack has completely released the page, in
> order to prevent use-after-free or modification of data pages which are still
> in use by the stack.
>
> It is allowable (indeed expected) for a caller to share a single destructor
> instance between multiple pages injected into the stack e.g. a group of pages
> included in a single higher level operation might share a destructor which is
> used to complete that higher level operation.
>
> With this change and the previous two changes to shinfo alignment and field
> orderring it is now the case tyhat on a 64 bit system with 64 byte cache lines,
^^^^ - that.
> everything from nr_frags until the end of frags[0] is on the same cacheline.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
> Cc: netdev@vger.kernel.org
> ---
> include/linux/skbuff.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> net/core/skbuff.c | 17 +++++++++++++++++
> 2 files changed, 60 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f0ae39c..6ac283e 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -166,9 +166,15 @@ struct sk_buff;
>
> typedef struct skb_frag_struct skb_frag_t;
>
> +struct skb_frag_destructor {
> + atomic_t ref;
> + int (*destroy)(struct skb_frag_destructor *destructor);
> +};
> +
> struct skb_frag_struct {
> struct {
> struct page *p;
> + struct skb_frag_destructor *destructor;
> } page;
> #if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536)
> __u32 page_offset;
> @@ -1221,6 +1227,31 @@ static inline int skb_pagelen(const struct sk_buff *skb)
> }
>
> /**
> + * skb_frag_set_destructor - set destructor for a paged fragment
> + * @skb: buffer containing fragment to be initialised
> + * @i: paged fragment index to initialise
> + * @destroy: the destructor to use for this fragment
> + *
> + * Sets @destroy as the destructor to be called when all references to
> + * the frag @i in @skb (tracked over skb_clone, retransmit, pull-ups,
> + * etc) are released.
> + *
> + * When a destructor is set then reference counting is performed on
> + * @destroy->ref. When the ref reaches zero then @destroy->destroy
> + * will be called. The caller is responsible for holding and managing
> + * any other references (such a the struct page reference count).
> + *
> + * This function must be called before any use of skb_frag_ref() or
> + * skb_frag_unref().
> + */
> +static inline void skb_frag_set_destructor(struct sk_buff *skb, int i,
> + struct skb_frag_destructor *destroy)
> +{
> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> + frag->page.destructor = destroy;
> +}
> +
> +/**
> * __skb_fill_page_desc - initialise a paged fragment in an skb
> * @skb: buffer containing fragment to be initialised
> * @i: paged fragment index to initialise
> @@ -1239,6 +1270,7 @@ static inline void __skb_fill_page_desc(struct sk_buff *skb, int i,
> skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>
> frag->page.p = page;
> + frag->page.destructor = NULL;
> frag->page_offset = off;
> skb_frag_size_set(frag, size);
> }
> @@ -1743,6 +1775,9 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
> return frag->page.p;
> }
>
> +extern void skb_frag_destructor_ref(struct skb_frag_destructor *destroy);
> +extern void skb_frag_destructor_unref(struct skb_frag_destructor *destroy);
> +
> /**
> * __skb_frag_ref - take an addition reference on a paged fragment.
> * @frag: the paged fragment
> @@ -1751,6 +1786,10 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
> */
> static inline void __skb_frag_ref(skb_frag_t *frag)
> {
> + if (unlikely(frag->page.destructor)) {
> + skb_frag_destructor_ref(frag->page.destructor);
> + return;
> + }
> get_page(skb_frag_page(frag));
> }
>
> @@ -1774,6 +1813,10 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f)
> */
> static inline void __skb_frag_unref(skb_frag_t *frag)
> {
> + if (unlikely(frag->page.destructor)) {
> + skb_frag_destructor_unref(frag->page.destructor);
> + return;
> + }
> put_page(skb_frag_page(frag));
> }
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b8a41d6..9ec88ce 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -349,6 +349,23 @@ struct sk_buff *dev_alloc_skb(unsigned int length)
> }
> EXPORT_SYMBOL(dev_alloc_skb);
>
> +void skb_frag_destructor_ref(struct skb_frag_destructor *destroy)
> +{
> + BUG_ON(destroy == NULL);
> + atomic_inc(&destroy->ref);
> +}
> +EXPORT_SYMBOL(skb_frag_destructor_ref);
> +
> +void skb_frag_destructor_unref(struct skb_frag_destructor *destroy)
> +{
> + if (destroy == NULL)
> + return;
> +
> + if (atomic_dec_and_test(&destroy->ref))
> + destroy->destroy(destroy);
> +}
> +EXPORT_SYMBOL(skb_frag_destructor_unref);
> +
> static void skb_drop_list(struct sk_buff **listp)
> {
> struct sk_buff *list = *listp;
> --
> 1.7.2.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply
* [PATCH net-next] bonding: bond_update_speed_duplex() can return void since no callers check its return
From: Rick Jones @ 2012-04-26 21:20 UTC (permalink / raw)
To: netdev; +Cc: fubar
From: Rick Jones <rick.jones2@hp.com>
As none of the callers of bond_update_speed_duplex (need to) check its
return value, there is little point in it returning anything.
Signed-off-by: Rick Jones <rick.jones2@hp.com>
---
Compile tested only
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 44e6a64..16dbf53 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -549,9 +549,9 @@ down:
* Get link speed and duplex from the slave's base driver
* using ethtool. If for some reason the call fails or the
* values are invalid, set speed and duplex to -1,
- * and return error.
+ * and return.
*/
-static int bond_update_speed_duplex(struct slave *slave)
+static void bond_update_speed_duplex(struct slave *slave)
{
struct net_device *slave_dev = slave->dev;
struct ethtool_cmd ecmd;
@@ -563,24 +563,24 @@ static int bond_update_speed_duplex(struct slave *slave)
res = __ethtool_get_settings(slave_dev, &ecmd);
if (res < 0)
- return -1;
+ return;
slave_speed = ethtool_cmd_speed(&ecmd);
if (slave_speed == 0 || slave_speed == ((__u32) -1))
- return -1;
+ return;
switch (ecmd.duplex) {
case DUPLEX_FULL:
case DUPLEX_HALF:
break;
default:
- return -1;
+ return;
}
slave->speed = slave_speed;
slave->duplex = ecmd.duplex;
- return 0;
+ return;
}
/*
^ permalink raw reply related
* [PATCH v3 0/2] fix problem with static_branch() for sock memcg
From: Glauber Costa @ 2012-04-26 21:24 UTC (permalink / raw)
To: cgroups; +Cc: netdev, Li Zefan, Tejun Heo, kamezawa.hiroyu, linux-mm, devel
Hi,
While trying to fulfill's Christoph's request for using static_branches
to do part of the role of number_of_cpusets in the cpuset cgroup, I took
a much more extensive look at the cpuset code (Thanks Christoph).
I started to feel that removing the cgroup_lock() from cpuset's
destroy is not as safe as I first imagined. At the very best, is not safe
enough to be bundled in a bugfix and deserves its own analysis.
I started then to consider another approach. While I voiced many times
that I would not like to do deferred updates for the static_branches, doing
that during destroy time would be perfectly acceptable IMHO (creation is
another story). In a summary, we are effectively calling the static_branch
updates only when the last reference to the memcg is gone. And that is
already asynchronous by nature, and we cope well with that.
In memcg, it turns out that we already do deferred freeing of the memcg
structure depending on the size of struct mem_cgroup.
My proposal is to always do that, and then we get a worker more or less
for free. Patch 2 is basically the same I had posted before, with minor
adaptations, plus the addition of a commentary explaining a race as
requested by Kame.
Let me know if this is acceptable.
Thanks
Glauber Costa (2):
Always free struct memcg through schedule_work()
decrement static keys on real destroy time
include/net/sock.h | 9 ++++++
mm/memcontrol.c | 54 ++++++++++++++++++++++++++--------
net/ipv4/tcp_memcontrol.c | 70 ++++++++++++++++++++++++++++++++++++++++----
3 files changed, 113 insertions(+), 20 deletions(-)
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH v3 1/2] Always free struct memcg through schedule_work()
From: Glauber Costa @ 2012-04-26 21:24 UTC (permalink / raw)
To: cgroups
Cc: netdev, Li Zefan, Tejun Heo, kamezawa.hiroyu, linux-mm, devel,
Glauber Costa, Johannes Weiner, Michal Hocko
In-Reply-To: <1335475463-25167-1-git-send-email-glommer@parallels.com>
Right now we free struct memcg with kfree right after a
rcu grace period, but defer it if we need to use vfree() to get
rid of that memory area. We do that by need, because we need vfree
to be called in a process context.
This patch unifies this behavior, by ensuring that even kfree will
happen in a separate thread. The goal is to have a stable place to
call the upcoming jump label destruction function outside the realm
of the complicated and quite far-reaching cgroup lock (that can't be
held when calling neither the cpu_hotplug.lock nor the jump_label_mutex)
Signed-off-by: Glauber Costa <glommer@parallels.com>
CC: Tejun Heo <tj@kernel.org>
CC: Li Zefan <lizefan@huawei.com>
CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
CC: Johannes Weiner <hannes@cmpxchg.org>
CC: Michal Hocko <mhocko@suse.cz>
---
mm/memcontrol.c | 24 +++++++++++++-----------
1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7832b4d..b0076cc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -245,8 +245,8 @@ struct mem_cgroup {
*/
struct rcu_head rcu_freeing;
/*
- * But when using vfree(), that cannot be done at
- * interrupt time, so we must then queue the work.
+ * We also need some space for a worker in deferred freeing.
+ * By the time we call it, rcu_freeing is not longer in use.
*/
struct work_struct work_freeing;
};
@@ -4826,23 +4826,28 @@ out_free:
}
/*
- * Helpers for freeing a vzalloc()ed mem_cgroup by RCU,
+ * Helpers for freeing a kmalloc()ed/vzalloc()ed mem_cgroup by RCU,
* but in process context. The work_freeing structure is overlaid
* on the rcu_freeing structure, which itself is overlaid on memsw.
*/
-static void vfree_work(struct work_struct *work)
+static void free_work(struct work_struct *work)
{
struct mem_cgroup *memcg;
+ int size = sizeof(struct mem_cgroup);
memcg = container_of(work, struct mem_cgroup, work_freeing);
- vfree(memcg);
+ if (size < PAGE_SIZE)
+ kfree(memcg);
+ else
+ vfree(memcg);
}
-static void vfree_rcu(struct rcu_head *rcu_head)
+
+static void free_rcu(struct rcu_head *rcu_head)
{
struct mem_cgroup *memcg;
memcg = container_of(rcu_head, struct mem_cgroup, rcu_freeing);
- INIT_WORK(&memcg->work_freeing, vfree_work);
+ INIT_WORK(&memcg->work_freeing, free_work);
schedule_work(&memcg->work_freeing);
}
@@ -4868,10 +4873,7 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg)
free_mem_cgroup_per_zone_info(memcg, node);
free_percpu(memcg->stat);
- if (sizeof(struct mem_cgroup) < PAGE_SIZE)
- kfree_rcu(memcg, rcu_freeing);
- else
- call_rcu(&memcg->rcu_freeing, vfree_rcu);
+ call_rcu(&memcg->rcu_freeing, free_rcu);
}
static void mem_cgroup_get(struct mem_cgroup *memcg)
--
1.7.7.6
^ permalink raw reply related
* [PATCH v3 2/2] decrement static keys on real destroy time
From: Glauber Costa @ 2012-04-26 21:24 UTC (permalink / raw)
To: cgroups
Cc: netdev, Li Zefan, Tejun Heo, kamezawa.hiroyu, linux-mm, devel,
Glauber Costa
In-Reply-To: <1335475463-25167-1-git-send-email-glommer@parallels.com>
We call the destroy function when a cgroup starts to be removed,
such as by a rmdir event.
However, because of our reference counters, some objects are still
inflight. Right now, we are decrementing the static_keys at destroy()
time, meaning that if we get rid of the last static_key reference,
some objects will still have charges, but the code to properly
uncharge them won't be run.
This becomes a problem specially if it is ever enabled again, because
now new charges will be added to the staled charges making keeping
it pretty much impossible.
We just need to be careful with the static branch activation:
since there is no particular preferred order of their activation,
we need to make sure that we only start using it after all
call sites are active. This is achieved by having a per-memcg
flag that is only updated after static_key_slow_inc() returns.
At this time, we are sure all sites are active.
This is made per-memcg, not global, for a reason:
it also has the effect of making socket accounting more
consistent. The first memcg to be limited will trigger static_key()
activation, therefore, accounting. But all the others will then be
accounted no matter what. After this patch, only limited memcgs
will have its sockets accounted.
[v2: changed a tcp limited flag for a generic proto limited flag ]
[v3: update the current active flag only after the static_key update ]
[v4: disarm_static_keys() inside free_work ]
Signed-off-by: Glauber Costa <glommer@parallels.com>
---
include/net/sock.h | 9 ++++++
mm/memcontrol.c | 31 ++++++++++++++++++-
net/ipv4/tcp_memcontrol.c | 70 ++++++++++++++++++++++++++++++++++++++++----
3 files changed, 101 insertions(+), 9 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index b3ebe6b..c5a2010 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -914,6 +914,15 @@ struct cg_proto {
int *memory_pressure;
long *sysctl_mem;
/*
+ * active means it is currently active, and new sockets should
+ * be assigned to cgroups.
+ *
+ * activated means it was ever activated, and we need to
+ * disarm the static keys on destruction
+ */
+ bool activated;
+ bool active;
+ /*
* memcg field is used to find which memcg we belong directly
* Each memcg struct can hold more than one cg_proto, so container_of
* won't really cut.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b0076cc..53a0815 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -404,6 +404,7 @@ void sock_update_memcg(struct sock *sk)
{
if (mem_cgroup_sockets_enabled) {
struct mem_cgroup *memcg;
+ struct cg_proto *cg_proto;
BUG_ON(!sk->sk_prot->proto_cgroup);
@@ -423,9 +424,10 @@ void sock_update_memcg(struct sock *sk)
rcu_read_lock();
memcg = mem_cgroup_from_task(current);
- if (!mem_cgroup_is_root(memcg)) {
+ cg_proto = sk->sk_prot->proto_cgroup(memcg);
+ if (!mem_cgroup_is_root(memcg) && cg_proto->active) {
mem_cgroup_get(memcg);
- sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg);
+ sk->sk_cgrp = cg_proto;
}
rcu_read_unlock();
}
@@ -442,6 +444,14 @@ void sock_release_memcg(struct sock *sk)
}
}
+static void disarm_static_keys(struct mem_cgroup *memcg)
+{
+#ifdef CONFIG_INET
+ if (memcg->tcp_mem.cg_proto.activated)
+ static_key_slow_dec(&memcg_socket_limit_enabled);
+#endif
+}
+
#ifdef CONFIG_INET
struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
{
@@ -452,6 +462,11 @@ struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
}
EXPORT_SYMBOL(tcp_proto_cgroup);
#endif /* CONFIG_INET */
+#else
+static inline void disarm_static_keys(struct mem_cgroup *memcg)
+{
+}
+
#endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */
static void drain_all_stock_async(struct mem_cgroup *memcg);
@@ -4836,6 +4851,18 @@ static void free_work(struct work_struct *work)
int size = sizeof(struct mem_cgroup);
memcg = container_of(work, struct mem_cgroup, work_freeing);
+ /*
+ * We need to make sure that (at least for now), the jump label
+ * destruction code runs outside of the cgroup lock. It is in theory
+ * possible to call the cgroup destruction function outside of that
+ * lock, but it is not yet done. rate limiting plus the deferred
+ * interface for static_branch destruction guarantees that it will
+ * run through schedule_work(), therefore, not holding any cgroup
+ * related lock (this is, of course, until someone decides to write
+ * a schedule_work cgroup :p )
+ */
+
+ disarm_static_keys(memcg);
if (size < PAGE_SIZE)
kfree(memcg);
else
diff --git a/net/ipv4/tcp_memcontrol.c b/net/ipv4/tcp_memcontrol.c
index 1517037..7790008 100644
--- a/net/ipv4/tcp_memcontrol.c
+++ b/net/ipv4/tcp_memcontrol.c
@@ -54,6 +54,8 @@ int tcp_init_cgroup(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
cg_proto->sysctl_mem = tcp->tcp_prot_mem;
cg_proto->memory_allocated = &tcp->tcp_memory_allocated;
cg_proto->sockets_allocated = &tcp->tcp_sockets_allocated;
+ cg_proto->active = false;
+ cg_proto->activated = false;
cg_proto->memcg = memcg;
return 0;
@@ -74,12 +76,43 @@ void tcp_destroy_cgroup(struct mem_cgroup *memcg)
percpu_counter_destroy(&tcp->tcp_sockets_allocated);
val = res_counter_read_u64(&tcp->tcp_memory_allocated, RES_LIMIT);
-
- if (val != RESOURCE_MAX)
- static_key_slow_dec(&memcg_socket_limit_enabled);
}
EXPORT_SYMBOL(tcp_destroy_cgroup);
+/*
+ * This is to prevent two writes arriving at the same time
+ * at kmem.tcp.limit_in_bytes.
+ *
+ * There is a race at the first time we write to this file:
+ *
+ * - cg_proto->activated == false for all writers.
+ * - They all do a static_key_slow_inc().
+ * - When we are finally read to decrement the static_keys,
+ * we'll do it only once per activated cgroup. So we won't
+ * be able to disable it.
+ *
+ * Also, after the first caller increments the static_branch
+ * counter, all others will return right away. That does not mean,
+ * however, that the update is finished.
+ *
+ * Without this mutex, it would then be possible for a second writer
+ * to get to the update site, return
+ *
+ * When a user updates limit of 2 cgroups at once, following happens.
+ *
+ * CPU A CPU B
+ *
+ * if (cg_proto->activated) if (cg->proto_activated)
+ * static_key_inc() static_key_inc()
+ * => set counter 0->1 => set counter 1->2,
+ * return immediately.
+ * => hold mutex => cg_proto->activated = true.
+ * => overwrite jmps.
+ *
+ * This race was described by Kamezawa Hiroyuki.
+ */
+static DEFINE_MUTEX(tcp_set_limit_mutex);
+
static int tcp_update_limit(struct mem_cgroup *memcg, u64 val)
{
struct net *net = current->nsproxy->net_ns;
@@ -107,10 +140,33 @@ static int tcp_update_limit(struct mem_cgroup *memcg, u64 val)
tcp->tcp_prot_mem[i] = min_t(long, val >> PAGE_SHIFT,
net->ipv4.sysctl_tcp_mem[i]);
- if (val == RESOURCE_MAX && old_lim != RESOURCE_MAX)
- static_key_slow_dec(&memcg_socket_limit_enabled);
- else if (old_lim == RESOURCE_MAX && val != RESOURCE_MAX)
- static_key_slow_inc(&memcg_socket_limit_enabled);
+ if (val == RESOURCE_MAX)
+ cg_proto->active = false;
+ else if (val != RESOURCE_MAX) {
+ /*
+ * ->activated needs to be written after the static_key update.
+ * This is what guarantees that the socket activation function
+ * is the last one to run. See sock_update_memcg() for details,
+ * and note that we don't mark any socket as belonging to this
+ * memcg until that flag is up.
+ *
+ * We need to do this, because static_keys will span multiple
+ * sites, but we can't control their order. If we mark a socket
+ * as accounted, but the accounting functions are not patched in
+ * yet, we'll lose accounting.
+ *
+ * We never race with the readers in sock_update_memcg(), because
+ * when this value change, the code to process it is not patched in
+ * yet.
+ */
+ mutex_lock(&tcp_set_limit_mutex);
+ if (!cg_proto->activated) {
+ static_key_slow_inc(&memcg_socket_limit_enabled);
+ cg_proto->activated = true;
+ }
+ mutex_unlock(&tcp_set_limit_mutex);
+ cg_proto->active = true;
+ }
return 0;
}
--
1.7.7.6
^ permalink raw reply related
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Tejun Heo @ 2012-04-26 21:39 UTC (permalink / raw)
To: Glauber Costa
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
Li Zefan, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <1335475463-25167-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Hello, Glauber.
Overall, I like this approach much better. Just some nits below.
On Thu, Apr 26, 2012 at 06:24:23PM -0300, Glauber Costa wrote:
> @@ -4836,6 +4851,18 @@ static void free_work(struct work_struct *work)
> int size = sizeof(struct mem_cgroup);
>
> memcg = container_of(work, struct mem_cgroup, work_freeing);
> + /*
> + * We need to make sure that (at least for now), the jump label
> + * destruction code runs outside of the cgroup lock. It is in theory
> + * possible to call the cgroup destruction function outside of that
> + * lock, but it is not yet done. rate limiting plus the deferred
> + * interface for static_branch destruction guarantees that it will
> + * run through schedule_work(), therefore, not holding any cgroup
> + * related lock (this is, of course, until someone decides to write
> + * a schedule_work cgroup :p )
> + */
Isn't the above a bit too verbose? Wouldn't just stating the locking
dependency be enough?
> + disarm_static_keys(memcg);
> if (size < PAGE_SIZE)
> kfree(memcg);
> else
> diff --git a/net/ipv4/tcp_memcontrol.c b/net/ipv4/tcp_memcontrol.c
> index 1517037..7790008 100644
> --- a/net/ipv4/tcp_memcontrol.c
> +++ b/net/ipv4/tcp_memcontrol.c
> @@ -54,6 +54,8 @@ int tcp_init_cgroup(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
> cg_proto->sysctl_mem = tcp->tcp_prot_mem;
> cg_proto->memory_allocated = &tcp->tcp_memory_allocated;
> cg_proto->sockets_allocated = &tcp->tcp_sockets_allocated;
> + cg_proto->active = false;
> + cg_proto->activated = false;
Isn't the memory zallocd? I find 0 / NULL / false inits unnecessary
and even misleading (can the memory be non-zero here?). Another side
effect is that it tends to get out of sync as more fields are added.
> +/*
> + * This is to prevent two writes arriving at the same time
> + * at kmem.tcp.limit_in_bytes.
> + *
> + * There is a race at the first time we write to this file:
> + *
> + * - cg_proto->activated == false for all writers.
> + * - They all do a static_key_slow_inc().
> + * - When we are finally read to decrement the static_keys,
^
ready
> + * we'll do it only once per activated cgroup. So we won't
> + * be able to disable it.
> + *
> + * Also, after the first caller increments the static_branch
> + * counter, all others will return right away. That does not mean,
> + * however, that the update is finished.
> + *
> + * Without this mutex, it would then be possible for a second writer
> + * to get to the update site, return
I kinda don't follow the above sentence.
> + * When a user updates limit of 2 cgroups at once, following happens.
> + *
> + * CPU A CPU B
> + *
> + * if (cg_proto->activated) if (cg->proto_activated)
> + * static_key_inc() static_key_inc()
> + * => set counter 0->1 => set counter 1->2,
> + * return immediately.
> + * => hold mutex => cg_proto->activated = true.
> + * => overwrite jmps.
Isn't this something which should be solved from static_keys API? Why
is this being worked around from memcg? Also, I again hope that the
explanation is slightly more concise.
Thanks.
--
tejun
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Shawn Bohrer @ 2012-04-26 21:44 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1335457888.2775.55.camel@edumazet-glaptop>
On Thu, Apr 26, 2012 at 06:31:28PM +0200, Eric Dumazet wrote:
> On Thu, 2012-04-26 at 11:28 -0500, Shawn Bohrer wrote:
>
> > No in this case it is 300 unique multicast addresses, and there is one
> > socket listening to each multicast address. So a single message is
> > only copied once to a single socket. The bottle neck appears to be
> > that even though a single message is only going to get copied to a
> > single socket we still have to walk the list of all 300 sockets while
> > holding the spin lock to figure that out. The incoming packet rate is
> > also roughly evenly distributed across all 300 multicast addresses so
> > even though we have multiple receive queues they are all contending
> > for the same spin lock.
> >
>
> I repeat my question : Are these 300 sockets bound to the same UDP
> port ?
>
> If not, they should be spreaded in hash table.
>
> You can make this hash table very big to reduce hash collisions
>
> Boot parameter : uhash_entries=65536
Thanks Eric I don't know how I missed this. In my test all 300
sockets were bound to the same UDP port so they were all falling into
the same bucket. Switching the test to use unique ports solves the
issue.
I didn't try your other patch to increase the stack size up to 512
sockets because I don't think we need it. We rarely have more than a
single socket per machine receiving packets on a multicast address so
I think the current stack size is sufficient for us. Or perhaps once
again I may be misunderstanding the purpose of that patch.
--
Shawn
--
---------------------------------------------------------------
This email, along with any attachments, is confidential. If you
believe you received this message in error, please contact the
sender immediately and delete all copies of the message.
Thank you.
^ permalink raw reply
* [PATCH] NET: smsc-ircc2: mark non-experimental
From: Linus Walleij @ 2012-04-26 21:51 UTC (permalink / raw)
To: netdev; +Cc: Linus Walleij
From: Linus Walleij <linus.walleij@linaro.org>
This has been used by me and others for ages, let's stop calling it
experimental.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/irda/Kconfig | 4 ++--
drivers/net/irda/smsc-ircc2.c | 1 -
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/irda/Kconfig b/drivers/net/irda/Kconfig
index 4680478..3575844 100644
--- a/drivers/net/irda/Kconfig
+++ b/drivers/net/irda/Kconfig
@@ -321,8 +321,8 @@ config AU1000_FIR
Say M to build a module; it will be called au1k_ir.ko
config SMC_IRCC_FIR
- tristate "SMSC IrCC (EXPERIMENTAL)"
- depends on EXPERIMENTAL && IRDA && ISA_DMA_API
+ tristate "SMSC IrCC"
+ depends on IRDA && ISA_DMA_API
help
Say Y here if you want to build support for the SMC Infrared
Communications Controller. It is used in a wide variety of
diff --git a/drivers/net/irda/smsc-ircc2.c b/drivers/net/irda/smsc-ircc2.c
index 6c95d40..a926813 100644
--- a/drivers/net/irda/smsc-ircc2.c
+++ b/drivers/net/irda/smsc-ircc2.c
@@ -1,7 +1,6 @@
/*********************************************************************
*
* Description: Driver for the SMC Infrared Communications Controller
- * Status: Experimental.
* Author: Daniele Peri (peri@csai.unipa.it)
* Created at:
* Modified at:
--
1.7.9.2
^ permalink raw reply related
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Glauber Costa @ 2012-04-26 21:58 UTC (permalink / raw)
To: Tejun Heo; +Cc: cgroups, netdev, Li Zefan, kamezawa.hiroyu, linux-mm, devel
In-Reply-To: <20120426213916.GD27486@google.com>
On 04/26/2012 06:39 PM, Tejun Heo wrote:
> Hello, Glauber.
>
> Overall, I like this approach much better. Just some nits below.
>
> On Thu, Apr 26, 2012 at 06:24:23PM -0300, Glauber Costa wrote:
>> @@ -4836,6 +4851,18 @@ static void free_work(struct work_struct *work)
>> int size = sizeof(struct mem_cgroup);
>>
>> memcg = container_of(work, struct mem_cgroup, work_freeing);
>> + /*
>> + * We need to make sure that (at least for now), the jump label
>> + * destruction code runs outside of the cgroup lock. It is in theory
>> + * possible to call the cgroup destruction function outside of that
>> + * lock, but it is not yet done. rate limiting plus the deferred
>> + * interface for static_branch destruction guarantees that it will
>> + * run through schedule_work(), therefore, not holding any cgroup
>> + * related lock (this is, of course, until someone decides to write
>> + * a schedule_work cgroup :p )
>> + */
>
> Isn't the above a bit too verbose? Wouldn't just stating the locking
> dependency be enough?
I used a lot of verbosity here because it is a tricky and racy issue.
I am fine with trimming the comments if this is considered too much.
>> cg_proto->sysctl_mem = tcp->tcp_prot_mem;
>> cg_proto->memory_allocated =&tcp->tcp_memory_allocated;
>> cg_proto->sockets_allocated =&tcp->tcp_sockets_allocated;
>> + cg_proto->active = false;
>> + cg_proto->activated = false;
>
> Isn't the memory zallocd? I find 0 / NULL / false inits unnecessary
> and even misleading (can the memory be non-zero here?). Another side
> effect is that it tends to get out of sync as more fields are added.
I can take them off.
>
>> +/*
>> + * This is to prevent two writes arriving at the same time
>> + * at kmem.tcp.limit_in_bytes.
>> + *
>> + * There is a race at the first time we write to this file:
>> + *
>> + * - cg_proto->activated == false for all writers.
>> + * - They all do a static_key_slow_inc().
>> + * - When we are finally read to decrement the static_keys,
> ^
> ready
Thanks.
>> + * we'll do it only once per activated cgroup. So we won't
>> + * be able to disable it.
>> + *
>> + * Also, after the first caller increments the static_branch
>> + * counter, all others will return right away. That does not mean,
>> + * however, that the update is finished.
>> + *
>> + * Without this mutex, it would then be possible for a second writer
>> + * to get to the update site, return
>
> I kinda don't follow the above sentence.
I will try to rephrase it for more clarity. But this is the thing behind
this patchset coming and going with so many attempts:
jump label updates are atomic given a single patch site. But they are
*not* atomic given multiple patch sites.
In our case, they are pretty spread around. Which means that while some
of them are already patched, some are not. If the socket marking in
sock_update_memcg is done last, we're fine, because all the accounters
test for that. Otherwise, we can misaccount.
To protect against that, we use the "activated" field. But it need to be
lock-protected, otherwise a second writer can arrive here before the
update is finished, update the accounted field, and we're down to the
same problem as before.
>> + * When a user updates limit of 2 cgroups at once, following happens.
>> + *
>> + * CPU A CPU B
>> + *
>> + * if (cg_proto->activated) if (cg->proto_activated)
>> + * static_key_inc() static_key_inc()
>> + * => set counter 0->1 => set counter 1->2,
>> + * return immediately.
>> + * => hold mutex => cg_proto->activated = true.
>> + * => overwrite jmps.
>
> Isn't this something which should be solved from static_keys API? Why
> is this being worked around from memcg? Also, I again hope that the
> explanation is slightly more concise.
>
At first I though that we could get rid of all this complication by
calling stop machine from the static_branch API. This would all
magically go away. I actually even tried it.
However, reading the code for other architectures (other than x86), I
found that they usually rely on the fixed instruction size to just patch
an instruction atomically and go home happy.
Using stop machine and the like would slow them down considerably. Not
only slow down the static branch update (which is acceptable), but
everybody else (which is horrible). It seemed to defeat the purpose of
static branches a bit.
The other users of static branches seems to be fine coping with the fact
that in cases with multiple-sites, they will spread in time.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Tejun Heo @ 2012-04-26 22:13 UTC (permalink / raw)
To: Glauber Costa; +Cc: cgroups, netdev, Li Zefan, kamezawa.hiroyu, linux-mm, devel
In-Reply-To: <4F99C50D.6070503@parallels.com>
Hello, Glauber.
On Thu, Apr 26, 2012 at 06:58:37PM -0300, Glauber Costa wrote:
> At first I though that we could get rid of all this complication by
> calling stop machine from the static_branch API. This would all
> magically go away. I actually even tried it.
>
> However, reading the code for other architectures (other than x86),
> I found that they usually rely on the fixed instruction size to just
> patch an instruction atomically and go home happy.
>
> Using stop machine and the like would slow them down considerably.
> Not only slow down the static branch update (which is acceptable),
> but everybody else (which is horrible). It seemed to defeat the
> purpose of static branches a bit.
>
> The other users of static branches seems to be fine coping with the
> fact that in cases with multiple-sites, they will spread in time.
No, what I mean is that why can't you do about the same mutexed
activated inside static_key API function instead of requiring every
user to worry about the function returning asynchronously.
ie. synchronize inside static_key API instead of in the callers.
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Glauber Costa @ 2012-04-26 22:17 UTC (permalink / raw)
To: Tejun Heo; +Cc: cgroups, netdev, Li Zefan, kamezawa.hiroyu, linux-mm, devel
In-Reply-To: <20120426221324.GE27486@google.com>
[-- Attachment #1: Type: text/plain, Size: 281 bytes --]
> No, what I mean is that why can't you do about the same mutexed
> activated inside static_key API function instead of requiring every
> user to worry about the function returning asynchronously.
> ie. synchronize inside static_key API instead of in the callers.
>
Like this?
[-- Attachment #2: jump_label.patch --]
[-- Type: text/x-patch, Size: 1360 bytes --]
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 4304919..f7cdc18 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -57,10 +57,11 @@ static void jump_label_update(struct static_key *key, int enable);
void static_key_slow_inc(struct static_key *key)
{
+ jump_label_lock();
+
if (atomic_inc_not_zero(&key->enabled))
- return;
+ goto out;
- jump_label_lock();
if (atomic_read(&key->enabled) == 0) {
if (!jump_label_get_branch_default(key))
jump_label_update(key, JUMP_LABEL_ENABLE);
@@ -68,6 +69,7 @@ void static_key_slow_inc(struct static_key *key)
jump_label_update(key, JUMP_LABEL_DISABLE);
}
atomic_inc(&key->enabled);
+out:
jump_label_unlock();
}
EXPORT_SYMBOL_GPL(static_key_slow_inc);
@@ -75,10 +77,11 @@ EXPORT_SYMBOL_GPL(static_key_slow_inc);
static void __static_key_slow_dec(struct static_key *key,
unsigned long rate_limit, struct delayed_work *work)
{
- if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) {
+ jump_label_lock();
+ if (atomic_dec_and_test(&key->enabled)) {
WARN(atomic_read(&key->enabled) < 0,
"jump label: negative count!\n");
- return;
+ goto out;
}
if (rate_limit) {
@@ -90,6 +93,8 @@ static void __static_key_slow_dec(struct static_key *key,
else
jump_label_update(key, JUMP_LABEL_ENABLE);
}
+
+out:
jump_label_unlock();
}
^ permalink raw reply related
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Tejun Heo @ 2012-04-26 22:22 UTC (permalink / raw)
To: Glauber Costa
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
Li Zefan, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <4F99C980.3030801-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Hello,
On Thu, Apr 26, 2012 at 3:17 PM, Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:
>
>> No, what I mean is that why can't you do about the same mutexed
>> activated inside static_key API function instead of requiring every
>> user to worry about the function returning asynchronously.
>> ie. synchronize inside static_key API instead of in the callers.
>>
>
> Like this?
Yeah, something like that. If keeping the inc operation a single
atomic op is important for performance or whatever reasons, you can
play some trick with large negative bias value while activation is
going on and use atomic_add_return() to determine both whether it's
the first incrementer and someone else is in the process of
activating.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Glauber Costa @ 2012-04-26 22:28 UTC (permalink / raw)
To: Tejun Heo; +Cc: cgroups, netdev, Li Zefan, kamezawa.hiroyu, linux-mm, devel
In-Reply-To: <CAOS58YOKUq7GTTZRcw19dth+HgThoNTEcqBKeNO0ftB4rFJ97A@mail.gmail.com>
On 04/26/2012 07:22 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Apr 26, 2012 at 3:17 PM, Glauber Costa<glommer@parallels.com> wrote:
>>
>>> No, what I mean is that why can't you do about the same mutexed
>>> activated inside static_key API function instead of requiring every
>>> user to worry about the function returning asynchronously.
>>> ie. synchronize inside static_key API instead of in the callers.
>>>
>>
>> Like this?
>
> Yeah, something like that. If keeping the inc operation a single
> atomic op is important for performance or whatever reasons, you can
> play some trick with large negative bias value while activation is
> going on and use atomic_add_return() to determine both whether it's
> the first incrementer and someone else is in the process of
> activating.
>
> Thanks.
>
We need a broader audience for this, but if I understand the interface
right, those functions should not be called in fast paths at all
(contrary to the static_branch tests)
The static_branch tests can be called from irq context, so we can't just
get rid of the atomic op and use the mutex everywhere, we'd have
to live with both.
I will repost this series, with some more people in the CC list.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 2/2] decrement static keys on real destroy time
From: Tejun Heo @ 2012-04-26 22:32 UTC (permalink / raw)
To: Glauber Costa
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
Li Zefan, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <4F99CC17.4080006-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
On Thu, Apr 26, 2012 at 3:28 PM, Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:
> We need a broader audience for this, but if I understand the interface
> right, those functions should not be called in fast paths at all (contrary
> to the static_branch tests)
>
> The static_branch tests can be called from irq context, so we can't just get
> rid of the atomic op and use the mutex everywhere, we'd have
> to live with both.
>
> I will repost this series, with some more people in the CC list.
Great, thanks!
--
tejun
^ 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