* Re: [PATCH 2/2] ARM: davinci: da850: add OF_DEV_AUXDATA entry for eth0.
From: Sekhar Nori @ 2013-01-25 6:25 UTC (permalink / raw)
To: Prabhakar Lad
Cc: LAK, DLOS, LKML, Lad, Prabhakar, netdev, devicetree-discuss,
Heiko Schocher
In-Reply-To: <1358749759-20517-3-git-send-email-prabhakar.lad@ti.com>
On 1/21/2013 11:59 AM, Prabhakar Lad wrote:
> From: Lad, Prabhakar <prabhakar.lad@ti.com>
>
> Add OF_DEV_AUXDATA for eth0 driver in da850 board dt
> file to use emac clock.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: davinci-linux-open-source@linux.davincidsp.com
> Cc: netdev@vger.kernel.org
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: Sekhar Nori <nsekhar@ti.com>
> Cc: Heiko Schocher <hs@denx.de>
> ---
> arch/arm/mach-davinci/da8xx-dt.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
> index 37c27af..d548a38 100644
> --- a/arch/arm/mach-davinci/da8xx-dt.c
> +++ b/arch/arm/mach-davinci/da8xx-dt.c
> @@ -37,11 +37,18 @@ static void __init da8xx_init_irq(void)
> of_irq_init(da8xx_irq_match);
> }
>
> +struct of_dev_auxdata da850_evm_auxdata_lookup[] __initdata = {
This is specific to da850 and not da850_evm so it should be named so.
You can call it da8xx_auxdata keeping with the file naming.
Thanks,
Sekhar
^ permalink raw reply
* Re: [patch net-next V2] bond: have random dev address by default instead of zeroes
From: Jiri Pirko @ 2013-01-25 6:37 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, davem, andy, stephen, psimerda, dcbw
In-Reply-To: <13906.1359077701@death.nxdomain>
Fri, Jan 25, 2013 at 02:35:01AM CET, fubar@us.ibm.com wrote:
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>>Makes more sense to have randomly generated address by default than to
>>have all zeroes. It also allows user to for example put the bond into
>>bridge without need to have any slaves in it.
>>
>>Also note that this changes only behaviour of bonds with no slaves. Once
>>the first slave device is enslaved, its address will be used (no change
>>here).
>>
>>Also, fix dev_assign_type values on the way.
>>
>>Reported-by: Pavel Šimerda <psimerda@redhat.com>
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
> Maybe I don't see it, but this feels like a bit of a hack just
>to get a bond with no slaves into a bridge. Am I missing something
>here? I just have this feeling that down the road I'm going to get
>questions as to why the bond gets a MAC, and then, poof, it vanishes
>when a slave is added. What's the point of the MAC address if it's only
>used to fool the bridge code?
I think that generally, the device should have valid mac address in
every point of its lifetime. I believe that bond is the only device who
behaves differently (bridge also uses random mac when no ports are
there)
>
> Also, when the bond's MAC changes from the random MAC to the
>first slave's MAC, does a notifier call need to happen? There isn't one
>now from the all zeroes to the first slave's, but that's from an invalid
>MAC to a valid one. There is already a notifier when the bond goes back
>to all zeroes, though.
You are right, all_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
should be called from bond_enslave() after calling bond_set_dev_addr()
>
> -J
>
>---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>
>>
>>v1->v2:
>>- fixed assign value of bond_dev->addr_assign_type in bond_set_dev_addr()
>>- added note to patch description
>>
>> drivers/net/bonding/bond_main.c | 28 ++++++++++++++++------------
>> 1 file changed, 16 insertions(+), 12 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 564cf42..1d56ac9 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1320,14 +1320,14 @@ static void bond_netpoll_cleanup(struct net_device *bond_dev)
>>
>> /*---------------------------------- IOCTL ----------------------------------*/
>>
>>-static int bond_sethwaddr(struct net_device *bond_dev,
>>- struct net_device *slave_dev)
>>+static void bond_set_dev_addr(struct net_device *bond_dev,
>>+ struct net_device *slave_dev)
>> {
>> pr_debug("bond_dev=%p\n", bond_dev);
>> pr_debug("slave_dev=%p\n", slave_dev);
>> pr_debug("slave_dev->addr_len=%d\n", slave_dev->addr_len);
>> memcpy(bond_dev->dev_addr, slave_dev->dev_addr, slave_dev->addr_len);
>>- return 0;
>>+ bond_dev->addr_assign_type = NET_ADDR_SET;
>> }
>>
>> static netdev_features_t bond_fix_features(struct net_device *dev,
>>@@ -1628,10 +1628,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>
>> /* If this is the first slave, then we need to set the master's hardware
>> * address to be the same as the slave's. */
>>- if (is_zero_ether_addr(bond->dev->dev_addr))
>>- memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
>>- slave_dev->addr_len);
>>-
>>+ if (bond->dev->addr_assign_type != NET_ADDR_SET)
>>+ bond_set_dev_addr(bond->dev, slave_dev);
>>
>> new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);
>> if (!new_slave) {
>>@@ -2049,11 +2047,11 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>> if (bond->slave_cnt == 0) {
>> bond_set_carrier(bond);
>>
>>- /* if the last slave was removed, zero the mac address
>>- * of the master so it will be set by the application
>>- * to the mac address of the first slave
>>+ /* If the last slave was removed, set random mac address
>>+ * of the master so it will be set by bond_enslave()
>>+ * to the mac address of the first slave.
>> */
>>- memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
>>+ eth_hw_addr_random(bond_dev);
>>
>> if (bond_vlan_used(bond)) {
>> pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
>>@@ -3708,7 +3706,8 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>> break;
>> case BOND_SETHWADDR_OLD:
>> case SIOCBONDSETHWADDR:
>>- res = bond_sethwaddr(bond_dev, slave_dev);
>>+ bond_set_dev_addr(bond_dev, slave_dev);
>>+ res = 0;
>> break;
>> case BOND_CHANGE_ACTIVE_OLD:
>> case SIOCBONDCHANGEACTIVE:
>>@@ -4858,6 +4857,11 @@ static int bond_init(struct net_device *bond_dev)
>>
>> bond_debug_register(bond);
>>
>>+ /* Ensure valid dev_addr */
>>+ if (is_zero_ether_addr(bond_dev->dev_addr) &&
>>+ bond_dev->addr_assign_type == NET_ADDR_PERM)
>>+ eth_hw_addr_random(bond_dev);
>>+
>> __hw_addr_init(&bond->mc_list);
>> return 0;
>> }
>>--
>>1.8.1
>>
>
^ permalink raw reply
* [patch] irda: buffer overflow in irnet_ctrl_read()
From: Dan Carpenter @ 2013-01-25 6:40 UTC (permalink / raw)
To: Samuel Ortiz; +Cc: David S. Miller, netdev, kernel-janitors
The comments here say that the /* Max event is 61 char */ but in 2003 we
changed the event format and now the max event size is 75. The longest
event is:
"Discovered %08x (%s) behind %08x {hints %02X-%02X}\n",
12345678901 23 456789012 34567890 1 2 3
+8 +21 +8 +2 +2 +1
= 75 characters.
There was a check to return -EOVERFLOW if the user gave us a "count"
value that was less than 64. Raising it to 75 might break backwards
compatability. Instead I removed the check and now it returns a
truncated string if "count" is too low.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/irda/irnet/irnet_ppp.c b/net/irda/irnet/irnet_ppp.c
index 2bb2beb..3c83a1e 100644
--- a/net/irda/irnet/irnet_ppp.c
+++ b/net/irda/irnet/irnet_ppp.c
@@ -214,8 +214,7 @@ irnet_get_discovery_log(irnet_socket * ap)
* After reading : discoveries = NULL ; disco_index = Y ; disco_number = -1
*/
static inline int
-irnet_read_discovery_log(irnet_socket * ap,
- char * event)
+irnet_read_discovery_log(irnet_socket *ap, char *event, int buf_size)
{
int done_event = 0;
@@ -237,12 +236,13 @@ irnet_read_discovery_log(irnet_socket * ap,
if(ap->disco_index < ap->disco_number)
{
/* Write an event */
- sprintf(event, "Found %08x (%s) behind %08x {hints %02X-%02X}\n",
- ap->discoveries[ap->disco_index].daddr,
- ap->discoveries[ap->disco_index].info,
- ap->discoveries[ap->disco_index].saddr,
- ap->discoveries[ap->disco_index].hints[0],
- ap->discoveries[ap->disco_index].hints[1]);
+ snprintf(event, buf_size,
+ "Found %08x (%s) behind %08x {hints %02X-%02X}\n",
+ ap->discoveries[ap->disco_index].daddr,
+ ap->discoveries[ap->disco_index].info,
+ ap->discoveries[ap->disco_index].saddr,
+ ap->discoveries[ap->disco_index].hints[0],
+ ap->discoveries[ap->disco_index].hints[1]);
DEBUG(CTRL_INFO, "Writing discovery %d : %s\n",
ap->disco_index, ap->discoveries[ap->disco_index].info);
@@ -282,27 +282,24 @@ irnet_ctrl_read(irnet_socket * ap,
size_t count)
{
DECLARE_WAITQUEUE(wait, current);
- char event[64]; /* Max event is 61 char */
+ char event[75];
ssize_t ret = 0;
DENTER(CTRL_TRACE, "(ap=0x%p, count=%Zd)\n", ap, count);
- /* Check if we can write an event out in one go */
- DABORT(count < sizeof(event), -EOVERFLOW, CTRL_ERROR, "Buffer to small.\n");
-
#ifdef INITIAL_DISCOVERY
/* Check if we have read the log */
- if(irnet_read_discovery_log(ap, event))
+ if (irnet_read_discovery_log(ap, event, sizeof(event)))
{
- /* We have an event !!! Copy it to the user */
- if(copy_to_user(buf, event, strlen(event)))
+ count = min(strlen(event), count);
+ if (copy_to_user(buf, event, count))
{
DERROR(CTRL_ERROR, "Invalid user space pointer.\n");
return -EFAULT;
}
DEXIT(CTRL_TRACE, "\n");
- return strlen(event);
+ return count;
}
#endif /* INITIAL_DISCOVERY */
@@ -339,79 +336,81 @@ irnet_ctrl_read(irnet_socket * ap,
switch(irnet_events.log[ap->event_index].event)
{
case IRNET_DISCOVER:
- sprintf(event, "Discovered %08x (%s) behind %08x {hints %02X-%02X}\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].saddr,
- irnet_events.log[ap->event_index].hints.byte[0],
- irnet_events.log[ap->event_index].hints.byte[1]);
+ snprintf(event, sizeof(event),
+ "Discovered %08x (%s) behind %08x {hints %02X-%02X}\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].saddr,
+ irnet_events.log[ap->event_index].hints.byte[0],
+ irnet_events.log[ap->event_index].hints.byte[1]);
break;
case IRNET_EXPIRE:
- sprintf(event, "Expired %08x (%s) behind %08x {hints %02X-%02X}\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].saddr,
- irnet_events.log[ap->event_index].hints.byte[0],
- irnet_events.log[ap->event_index].hints.byte[1]);
+ snprintf(event, sizeof(event),
+ "Expired %08x (%s) behind %08x {hints %02X-%02X}\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].saddr,
+ irnet_events.log[ap->event_index].hints.byte[0],
+ irnet_events.log[ap->event_index].hints.byte[1]);
break;
case IRNET_CONNECT_TO:
- sprintf(event, "Connected to %08x (%s) on ppp%d\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].unit);
+ snprintf(event, sizeof(event), "Connected to %08x (%s) on ppp%d\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].unit);
break;
case IRNET_CONNECT_FROM:
- sprintf(event, "Connection from %08x (%s) on ppp%d\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].unit);
+ snprintf(event, sizeof(event), "Connection from %08x (%s) on ppp%d\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].unit);
break;
case IRNET_REQUEST_FROM:
- sprintf(event, "Request from %08x (%s) behind %08x\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].saddr);
+ snprintf(event, sizeof(event), "Request from %08x (%s) behind %08x\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].saddr);
break;
case IRNET_NOANSWER_FROM:
- sprintf(event, "No-answer from %08x (%s) on ppp%d\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].unit);
+ snprintf(event, sizeof(event), "No-answer from %08x (%s) on ppp%d\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].unit);
break;
case IRNET_BLOCKED_LINK:
- sprintf(event, "Blocked link with %08x (%s) on ppp%d\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].unit);
+ snprintf(event, sizeof(event), "Blocked link with %08x (%s) on ppp%d\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].unit);
break;
case IRNET_DISCONNECT_FROM:
- sprintf(event, "Disconnection from %08x (%s) on ppp%d\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name,
- irnet_events.log[ap->event_index].unit);
+ snprintf(event, sizeof(event), "Disconnection from %08x (%s) on ppp%d\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name,
+ irnet_events.log[ap->event_index].unit);
break;
case IRNET_DISCONNECT_TO:
- sprintf(event, "Disconnected to %08x (%s)\n",
- irnet_events.log[ap->event_index].daddr,
- irnet_events.log[ap->event_index].name);
+ snprintf(event, sizeof(event), "Disconnected to %08x (%s)\n",
+ irnet_events.log[ap->event_index].daddr,
+ irnet_events.log[ap->event_index].name);
break;
default:
- sprintf(event, "Bug\n");
+ snprintf(event, sizeof(event), "Bug\n");
}
/* Increment our event index */
ap->event_index = (ap->event_index + 1) % IRNET_MAX_EVENTS;
DEBUG(CTRL_INFO, "Event is :%s", event);
- /* Copy it to the user */
- if(copy_to_user(buf, event, strlen(event)))
+ count = min(strlen(event), count);
+ if (copy_to_user(buf, event, count))
{
DERROR(CTRL_ERROR, "Invalid user space pointer.\n");
return -EFAULT;
}
DEXIT(CTRL_TRACE, "\n");
- return strlen(event);
+ return count;
}
/*------------------------------------------------------------------*/
^ permalink raw reply related
* [patch] qlcnic: silence false positive overflow warning
From: Dan Carpenter @ 2013-01-25 6:41 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: Sony Chacko, linux-driver, netdev, kernel-janitors
We actually store the MAC address as well as the board_name here. The
longest board_name is 75 characters so there is more than enough room
to hold the 17 character MAC and the ": " divider. But making this
buffer larger silences a static checker warning.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I'm not sure if this is worth sending. Feel free to drop this if you
want.
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 7f63b5f..09870d4 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -1724,7 +1724,7 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
struct qlcnic_adapter *adapter = NULL;
struct qlcnic_hardware_context *ahw;
int err, pci_using_dac = -1;
- char board_name[QLCNIC_MAX_BOARD_NAME_LEN];
+ char board_name[QLCNIC_MAX_BOARD_NAME_LEN + 19]; /* MAC + ": " + name */
err = pci_enable_device(pdev);
if (err)
^ permalink raw reply related
* Re: [PATCH V6 2/3] virtio-net: split out clean affinity function
From: Wanlong Gao @ 2013-01-25 6:42 UTC (permalink / raw)
To: Jason Wang
Cc: linux-kernel, Rusty Russell, Michael S. Tsirkin, Eric Dumazet,
virtualization, netdev, Wanlong Gao
In-Reply-To: <5102224C.3050909@redhat.com>
On 01/25/2013 02:12 PM, Jason Wang wrote:
> On 01/25/2013 01:40 PM, Wanlong Gao wrote:
>> On 01/25/2013 01:13 PM, Jason Wang wrote:
>>> On 01/25/2013 12:20 PM, Wanlong Gao wrote:
>>>> On 01/25/2013 11:28 AM, Jason Wang wrote:
>>>>> On 01/21/2013 07:25 PM, Wanlong Gao wrote:
>>>>>> Split out the clean affinity function to virtnet_clean_affinity().
>>>>>>
>>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>> Cc: Eric Dumazet <erdnetdev@gmail.com>
>>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>>> Cc: netdev@vger.kernel.org
>>>>>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>>>>> ---
>>>>>> V5->V6: NEW
>>>>>>
>>>>>> drivers/net/virtio_net.c | 67 +++++++++++++++++++++++++++---------------------
>>>>>> 1 file changed, 38 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>> index 70cd957..1a35a8c 100644
>>>>>> --- a/drivers/net/virtio_net.c
>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>> @@ -1016,48 +1016,57 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> -static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>>>>>> +static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
>>>>>> {
>>>>>> int i;
>>>>>> int cpu;
>>>>>>
>>>>>> - /* In multiqueue mode, when the number of cpu is equal to the number of
>>>>>> - * queue pairs, we let the queue pairs to be private to one cpu by
>>>>>> - * setting the affinity hint to eliminate the contention.
>>>>>> - */
>>>>>> - if ((vi->curr_queue_pairs == 1 ||
>>>>>> - vi->max_queue_pairs != num_online_cpus()) && set) {
>>>>>> - if (vi->affinity_hint_set)
>>>>>> - set = false;
>>>>>> - else
>>>>>> - return;
>>>>>> - }
>>>>>> -
>>>>>> - if (set) {
>>>>>> - i = 0;
>>>>>> - for_each_online_cpu(cpu) {
>>>>>> - virtqueue_/set_affinity(vi->rq[i].vq, cpu);
>>>>>> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>>>> - *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>>>> - i++;
>>>>>> - }
>>>>>> -
>>>>>> - vi->affinity_hint_set = true;
>>>>>> - } else {
>>>>>> - for(i = 0; i < vi->max_queue_pairs; i++) {
>>>>>> + if (vi->affinity_hint_set) {
>>>>>> + for (i = 0; i < vi->max_queue_pairs; i++) {
>>>>>> virtqueue_set_affinity(vi->rq[i].vq, -1);
>>>>>> virtqueue_set_affinity(vi->sq[i].vq, -1);
>>>>>> }
>>>>>>
>>>>>> i = 0;
>>>>>> - for_each_online_cpu(cpu)
>>>>>> + for_each_online_cpu(cpu) {
>>>>>> + if (cpu == hcpu)
>>>>>> + continue;
>>>>>> *per_cpu_ptr(vi->vq_index, cpu) =
>>>>>> ++i % vi->curr_queue_pairs;
>>>>>> + }
>>>>>>
>>>>> Some questions here:
>>>>>
>>>>> - Did we need reset the affinity of the queue here like the this?
>>>>>
>>>>> virtqueue_set_affinity(vi->sq[*per_cpu_ptr(vi->vq_index, hcpu)], -1);
>>>>> virtqueue_set_affinity(vi->rq[*per_cpu_ptr(vi->vq_index, hcpu)], -1);
>>>> I think no, we are going to unset the affinity of all the set queues,
>>>> include hcpu.
>>>>
>>>>> - Looks like we need also reset the percpu index when
>>>>> vi->affinity_hint_set is false.
>>>> Yes, follow this and the comment on [1/3].
>>>>
>>>>> - Does this really need this reset? Consider we're going to reset the
>>>>> percpu in CPU_DEAD?
>>>> I think resetting when CPU_DOWN_PREPARE can avoid selecting the wrong queue
>>>> on the dying CPU.
>>> Didn't understand this. What does 'wrong queue' here mean? Looks like
>>> you didn't change the preferable queue of the dying CPU and just change
>>> all others.
>> How about setting the vq index to -1 on hcpu when doing DOWN_PREPARE?
>> So that let it select txq to 0 when the CPU is dying.
>
> Looks safe, so look like what you're going to solve here is the the race
> between cpu hotplug and virtnet_set_channels(). A possible better
> solution is to serialize them by protecting virtnet_set_queues() by
> get_online_cpus() also. After this, we can make sure the number of
> channels were not changed during cpu hotplug, and looks like there's no
> need to reset the preferable queues in DOWN_PREPARE.
>
> What's your opinion?
IMHO, serialize every time will take lock and may slow down this path,
but the hot unplug path will be more cold than it. So I prefer reset the
preferable queues in DOWN_PREPARE but not serialize them. Agree?
Thanks,
Wanlong Gao
>
> Thanks
>>
>> Thanks,
>> Wanlong Gao
>>
>>>> Thanks,
>>>> Wanlong Gao
>>>>
>>>>> Thanks
>>>>>> vi->affinity_hint_set = false;
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +static void virtnet_set_affinity(struct virtnet_info *vi)
>>>>>> +{
>>>>>> + int i;
>>>>>> + int cpu;
>>>>>> +
>>>>>> + /* In multiqueue mode, when the number of cpu is equal to the number of
>>>>>> + * queue pairs, we let the queue pairs to be private to one cpu by
>>>>>> + * setting the affinity hint to eliminate the contention.
>>>>>> + */
>>>>>> + if (vi->curr_queue_pairs == 1 ||
>>>>>> + vi->max_queue_pairs != num_online_cpus()) {
>>>>>> + if (vi->affinity_hint_set)
>>>>>> + virtnet_clean_affinity(vi, -1);
>>>>>> + else
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + i = 0;
>>>>>> + for_each_online_cpu(cpu) {
>>>>>> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>>>>> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>>>> + *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>>>> + i++;
>>>>>> + }
>>>>>> +
>>>>>> + vi->affinity_hint_set = true;
>>>>>> +}
>>>>>> +
>>>>>> static void virtnet_get_ringparam(struct net_device *dev,
>>>>>> struct ethtool_ringparam *ring)
>>>>>> {
>>>>>> @@ -1105,7 +1114,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>>>>> netif_set_real_num_rx_queues(dev, queue_pairs);
>>>>>>
>>>>>> get_online_cpus();
>>>>>> - virtnet_set_affinity(vi, true);
>>>>>> + virtnet_set_affinity(vi);
>>>>>> put_online_cpus();
>>>>>> }
>>>>>>
>>>>>> @@ -1274,7 +1283,7 @@ static void virtnet_del_vqs(struct virtnet_info *vi)
>>>>>> {
>>>>>> struct virtio_device *vdev = vi->vdev;
>>>>>>
>>>>>> - virtnet_set_affinity(vi, false);
>>>>>> + virtnet_clean_affinity(vi, -1);
>>>>>>
>>>>>> vdev->config->del_vqs(vdev);
>>>>>>
>>>>>> @@ -1398,7 +1407,7 @@ static int init_vqs(struct virtnet_info *vi)
>>>>>> goto err_free;
>>>>>>
>>>>>> get_online_cpus();
>>>>>> - virtnet_set_affinity(vi, true);
>>>>>> + virtnet_set_affinity(vi);
>>>>>> put_online_cpus();
>>>>>>
>>>>>> return 0;
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
>
^ permalink raw reply
* [PATCH] tcp: Increment of LINUX_MIB_LISTENOVERFLOWS in tcp_v4_conn_request()
From: Nivedita Singhvi @ 2013-01-25 7:03 UTC (permalink / raw)
To: netdev, David Miller; +Cc: Leandro Lucarella, Nivedita Singhvi
We drop a connection request if the accept backlog, syn queue are full when processing reaches tcp_v4_conn_request(). Increment the appropriate counter so this isn't silent, for accurate stats and help in debugging.
Signed-off-by: Nivedita Singhvi <niv@us.ibm.com>
---
net/ipv4/tcp_ipv4.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index bbbdcc5..49b4f50 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1502,8 +1502,10 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
* clogging syn queue with openreqs with exponentially increasing
* timeout.
*/
- if (sk_acceptq_is_full(sk) && inet_csk_reqsk_queue_young(sk) > 1)
+ if (sk_acceptq_is_full(sk) && inet_csk_reqsk_queue_young(sk) > 1) {
+ NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
goto drop;
+ }
req = inet_reqsk_alloc(&tcp_request_sock_ops);
if (!req)
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH V6 2/3] virtio-net: split out clean affinity function
From: Jason Wang @ 2013-01-25 7:04 UTC (permalink / raw)
To: gaowanlong
Cc: Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <51022956.3050406@cn.fujitsu.com>
On 01/25/2013 02:42 PM, Wanlong Gao wrote:
> On 01/25/2013 02:12 PM, Jason Wang wrote:
>> On 01/25/2013 01:40 PM, Wanlong Gao wrote:
>>> On 01/25/2013 01:13 PM, Jason Wang wrote:
>>>> On 01/25/2013 12:20 PM, Wanlong Gao wrote:
>>>>> On 01/25/2013 11:28 AM, Jason Wang wrote:
>>>>>> On 01/21/2013 07:25 PM, Wanlong Gao wrote:
>>>>>>> Split out the clean affinity function to virtnet_clean_affinity().
>>>>>>>
>>>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>>> Cc: Eric Dumazet <erdnetdev@gmail.com>
>>>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>>>> Cc: netdev@vger.kernel.org
>>>>>>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>>>>>> ---
>>>>>>> V5->V6: NEW
>>>>>>>
>>>>>>> drivers/net/virtio_net.c | 67 +++++++++++++++++++++++++++---------------------
>>>>>>> 1 file changed, 38 insertions(+), 29 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>> index 70cd957..1a35a8c 100644
>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>> @@ -1016,48 +1016,57 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> -static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>>>>>>> +static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
>>>>>>> {
>>>>>>> int i;
>>>>>>> int cpu;
>>>>>>>
>>>>>>> - /* In multiqueue mode, when the number of cpu is equal to the number of
>>>>>>> - * queue pairs, we let the queue pairs to be private to one cpu by
>>>>>>> - * setting the affinity hint to eliminate the contention.
>>>>>>> - */
>>>>>>> - if ((vi->curr_queue_pairs == 1 ||
>>>>>>> - vi->max_queue_pairs != num_online_cpus()) && set) {
>>>>>>> - if (vi->affinity_hint_set)
>>>>>>> - set = false;
>>>>>>> - else
>>>>>>> - return;
>>>>>>> - }
>>>>>>> -
>>>>>>> - if (set) {
>>>>>>> - i = 0;
>>>>>>> - for_each_online_cpu(cpu) {
>>>>>>> - virtqueue_/set_affinity(vi->rq[i].vq, cpu);
>>>>>>> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>>>>> - *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>>>>> - i++;
>>>>>>> - }
>>>>>>> -
>>>>>>> - vi->affinity_hint_set = true;
>>>>>>> - } else {
>>>>>>> - for(i = 0; i < vi->max_queue_pairs; i++) {
>>>>>>> + if (vi->affinity_hint_set) {
>>>>>>> + for (i = 0; i < vi->max_queue_pairs; i++) {
>>>>>>> virtqueue_set_affinity(vi->rq[i].vq, -1);
>>>>>>> virtqueue_set_affinity(vi->sq[i].vq, -1);
>>>>>>> }
>>>>>>>
>>>>>>> i = 0;
>>>>>>> - for_each_online_cpu(cpu)
>>>>>>> + for_each_online_cpu(cpu) {
>>>>>>> + if (cpu == hcpu)
>>>>>>> + continue;
>>>>>>> *per_cpu_ptr(vi->vq_index, cpu) =
>>>>>>> ++i % vi->curr_queue_pairs;
>>>>>>> + }
>>>>>>>
>>>>>> Some questions here:
>>>>>>
>>>>>> - Did we need reset the affinity of the queue here like the this?
>>>>>>
>>>>>> virtqueue_set_affinity(vi->sq[*per_cpu_ptr(vi->vq_index, hcpu)], -1);
>>>>>> virtqueue_set_affinity(vi->rq[*per_cpu_ptr(vi->vq_index, hcpu)], -1);
>>>>> I think no, we are going to unset the affinity of all the set queues,
>>>>> include hcpu.
>>>>>
>>>>>> - Looks like we need also reset the percpu index when
>>>>>> vi->affinity_hint_set is false.
>>>>> Yes, follow this and the comment on [1/3].
>>>>>
>>>>>> - Does this really need this reset? Consider we're going to reset the
>>>>>> percpu in CPU_DEAD?
>>>>> I think resetting when CPU_DOWN_PREPARE can avoid selecting the wrong queue
>>>>> on the dying CPU.
>>>> Didn't understand this. What does 'wrong queue' here mean? Looks like
>>>> you didn't change the preferable queue of the dying CPU and just change
>>>> all others.
>>> How about setting the vq index to -1 on hcpu when doing DOWN_PREPARE?
>>> So that let it select txq to 0 when the CPU is dying.
>> Looks safe, so look like what you're going to solve here is the the race
>> between cpu hotplug and virtnet_set_channels(). A possible better
>> solution is to serialize them by protecting virtnet_set_queues() by
>> get_online_cpus() also. After this, we can make sure the number of
>> channels were not changed during cpu hotplug, and looks like there's no
>> need to reset the preferable queues in DOWN_PREPARE.
>>
>> What's your opinion?
> IMHO, serialize every time will take lock and may slow down this path,
> but the hot unplug path will be more cold than it. So I prefer reset the
> preferable queues in DOWN_PREPARE but not serialize them. Agree?
I think it's ok since we're in control path. And the point is when
you're trying to reset the affinity / preferable queues during cpu
hotplug callback, there will be another request in
virtnet_set_channels() which changing the number of queues. So the the
result of cpus == queues may out of date. Anyway you need some
synchronization.
>
> Thanks,
> Wanlong Gao
>
>> Thanks
>>> Thanks,
>>> Wanlong Gao
>>>
>>>>> Thanks,
>>>>> Wanlong Gao
>>>>>
>>>>>> Thanks
>>>>>>> vi->affinity_hint_set = false;
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> +static void virtnet_set_affinity(struct virtnet_info *vi)
>>>>>>> +{
>>>>>>> + int i;
>>>>>>> + int cpu;
>>>>>>> +
>>>>>>> + /* In multiqueue mode, when the number of cpu is equal to the number of
>>>>>>> + * queue pairs, we let the queue pairs to be private to one cpu by
>>>>>>> + * setting the affinity hint to eliminate the contention.
>>>>>>> + */
>>>>>>> + if (vi->curr_queue_pairs == 1 ||
>>>>>>> + vi->max_queue_pairs != num_online_cpus()) {
>>>>>>> + if (vi->affinity_hint_set)
>>>>>>> + virtnet_clean_affinity(vi, -1);
>>>>>>> + else
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + i = 0;
>>>>>>> + for_each_online_cpu(cpu) {
>>>>>>> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>>>>>> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>>>>> + *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>>>>> + i++;
>>>>>>> + }
>>>>>>> +
>>>>>>> + vi->affinity_hint_set = true;
>>>>>>> +}
>>>>>>> +
>>>>>>> static void virtnet_get_ringparam(struct net_device *dev,
>>>>>>> struct ethtool_ringparam *ring)
>>>>>>> {
>>>>>>> @@ -1105,7 +1114,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>>>>>> netif_set_real_num_rx_queues(dev, queue_pairs);
>>>>>>>
>>>>>>> get_online_cpus();
>>>>>>> - virtnet_set_affinity(vi, true);
>>>>>>> + virtnet_set_affinity(vi);
>>>>>>> put_online_cpus();
>>>>>>> }
>>>>>>>
>>>>>>> @@ -1274,7 +1283,7 @@ static void virtnet_del_vqs(struct virtnet_info *vi)
>>>>>>> {
>>>>>>> struct virtio_device *vdev = vi->vdev;
>>>>>>>
>>>>>>> - virtnet_set_affinity(vi, false);
>>>>>>> + virtnet_clean_affinity(vi, -1);
>>>>>>>
>>>>>>> vdev->config->del_vqs(vdev);
>>>>>>>
>>>>>>> @@ -1398,7 +1407,7 @@ static int init_vqs(struct virtnet_info *vi)
>>>>>>> goto err_free;
>>>>>>>
>>>>>>> get_online_cpus();
>>>>>>> - virtnet_set_affinity(vi, true);
>>>>>>> + virtnet_set_affinity(vi);
>>>>>>> put_online_cpus();
>>>>>>>
>>>>>>> return 0;
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] net: usbnet: prevent buggy devices from killing us
From: Bjørn Mork @ 2013-01-25 7:13 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-usb, netdev
In-Reply-To: <1794053.fjSaMDVud2@linux-5eaq.site>
Oliver Neukum <oliver@neukum.org> writes:
> On Thursday 24 January 2013 20:16:56 Bjørn Mork wrote:
>> A device sending 0 length frames as fast as it can has been
>> observed killing the host system due to the resulting memory
>> pressure.
>>
>> Temporarily disable RX skb allocation and URB submission when
>> the current error ratio is high, preventing us from trying to
>> allocate an infinite number of skbs. Reenable as soon as we
>> are finished processing the done queue, allowing the device
>> to continue working after short error bursts.
>>
>> Signed-off-by: Bjørn Mork <bjorn@mork.no>
>> ---
>> So is this starting to look OK?
>
> It seems to me that we at least need to try some error recovery.
Won't the disabling code in usbnet_bh do? RX will only stay disabled
until the done queue is handled.
> How about resetting the device when it is no longer used?
Yes, that we should do. I guess usbnet_open is the place to reset the
flag and counters? I'll send another version taking care of this and
Joes comment.
Bjørn
^ permalink raw reply
* Re: [PATCH V6 2/3] virtio-net: split out clean affinity function
From: Wanlong Gao @ 2013-01-25 7:22 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <51022E7E.3040905@redhat.com>
On 01/25/2013 03:04 PM, Jason Wang wrote:
> On 01/25/2013 02:42 PM, Wanlong Gao wrote:
>> On 01/25/2013 02:12 PM, Jason Wang wrote:
>>> On 01/25/2013 01:40 PM, Wanlong Gao wrote:
>>>> On 01/25/2013 01:13 PM, Jason Wang wrote:
>>>>> On 01/25/2013 12:20 PM, Wanlong Gao wrote:
>>>>>> On 01/25/2013 11:28 AM, Jason Wang wrote:
>>>>>>> On 01/21/2013 07:25 PM, Wanlong Gao wrote:
>>>>>>>> Split out the clean affinity function to virtnet_clean_affinity().
>>>>>>>>
>>>>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>>>> Cc: Eric Dumazet <erdnetdev@gmail.com>
>>>>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>>>>> Cc: netdev@vger.kernel.org
>>>>>>>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>>>>>>> ---
>>>>>>>> V5->V6: NEW
>>>>>>>>
>>>>>>>> drivers/net/virtio_net.c | 67 +++++++++++++++++++++++++++---------------------
>>>>>>>> 1 file changed, 38 insertions(+), 29 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>>> index 70cd957..1a35a8c 100644
>>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>>> @@ -1016,48 +1016,57 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>>>>>>>> +static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
>>>>>>>> {
>>>>>>>> int i;
>>>>>>>> int cpu;
>>>>>>>>
>>>>>>>> - /* In multiqueue mode, when the number of cpu is equal to the number of
>>>>>>>> - * queue pairs, we let the queue pairs to be private to one cpu by
>>>>>>>> - * setting the affinity hint to eliminate the contention.
>>>>>>>> - */
>>>>>>>> - if ((vi->curr_queue_pairs == 1 ||
>>>>>>>> - vi->max_queue_pairs != num_online_cpus()) && set) {
>>>>>>>> - if (vi->affinity_hint_set)
>>>>>>>> - set = false;
>>>>>>>> - else
>>>>>>>> - return;
>>>>>>>> - }
>>>>>>>> -
>>>>>>>> - if (set) {
>>>>>>>> - i = 0;
>>>>>>>> - for_each_online_cpu(cpu) {
>>>>>>>> - virtqueue_/set_affinity(vi->rq[i].vq, cpu);
>>>>>>>> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>>>>>> - *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>>>>>> - i++;
>>>>>>>> - }
>>>>>>>> -
>>>>>>>> - vi->affinity_hint_set = true;
>>>>>>>> - } else {
>>>>>>>> - for(i = 0; i < vi->max_queue_pairs; i++) {
>>>>>>>> + if (vi->affinity_hint_set) {
>>>>>>>> + for (i = 0; i < vi->max_queue_pairs; i++) {
>>>>>>>> virtqueue_set_affinity(vi->rq[i].vq, -1);
>>>>>>>> virtqueue_set_affinity(vi->sq[i].vq, -1);
>>>>>>>> }
>>>>>>>>
>>>>>>>> i = 0;
>>>>>>>> - for_each_online_cpu(cpu)
>>>>>>>> + for_each_online_cpu(cpu) {
>>>>>>>> + if (cpu == hcpu)
>>>>>>>> + continue;
>>>>>>>> *per_cpu_ptr(vi->vq_index, cpu) =
>>>>>>>> ++i % vi->curr_queue_pairs;
>>>>>>>> + }
>>>>>>>>
>>>>>>> Some questions here:
>>>>>>>
>>>>>>> - Did we need reset the affinity of the queue here like the this?
>>>>>>>
>>>>>>> virtqueue_set_affinity(vi->sq[*per_cpu_ptr(vi->vq_index, hcpu)], -1);
>>>>>>> virtqueue_set_affinity(vi->rq[*per_cpu_ptr(vi->vq_index, hcpu)], -1);
>>>>>> I think no, we are going to unset the affinity of all the set queues,
>>>>>> include hcpu.
>>>>>>
>>>>>>> - Looks like we need also reset the percpu index when
>>>>>>> vi->affinity_hint_set is false.
>>>>>> Yes, follow this and the comment on [1/3].
>>>>>>
>>>>>>> - Does this really need this reset? Consider we're going to reset the
>>>>>>> percpu in CPU_DEAD?
>>>>>> I think resetting when CPU_DOWN_PREPARE can avoid selecting the wrong queue
>>>>>> on the dying CPU.
>>>>> Didn't understand this. What does 'wrong queue' here mean? Looks like
>>>>> you didn't change the preferable queue of the dying CPU and just change
>>>>> all others.
>>>> How about setting the vq index to -1 on hcpu when doing DOWN_PREPARE?
>>>> So that let it select txq to 0 when the CPU is dying.
>>> Looks safe, so look like what you're going to solve here is the the race
>>> between cpu hotplug and virtnet_set_channels(). A possible better
>>> solution is to serialize them by protecting virtnet_set_queues() by
>>> get_online_cpus() also. After this, we can make sure the number of
>>> channels were not changed during cpu hotplug, and looks like there's no
>>> need to reset the preferable queues in DOWN_PREPARE.
>>>
>>> What's your opinion?
>> IMHO, serialize every time will take lock and may slow down this path,
>> but the hot unplug path will be more cold than it. So I prefer reset the
>> preferable queues in DOWN_PREPARE but not serialize them. Agree?
>
> I think it's ok since we're in control path. And the point is when
> you're trying to reset the affinity / preferable queues during cpu
> hotplug callback, there will be another request in
> virtnet_set_channels() which changing the number of queues. So the the
> result of cpus == queues may out of date. Anyway you need some
> synchronization.
Agree, then I will add {get|put}_online_cpus to serialize this, thank you.
Regards,
Wanlong Gao
>
>>
>> Thanks,
>> Wanlong Gao
>>
>>> Thanks
>>>> Thanks,
>>>> Wanlong Gao
>>>>
>>>>>> Thanks,
>>>>>> Wanlong Gao
>>>>>>
>>>>>>> Thanks
>>>>>>>> vi->affinity_hint_set = false;
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static void virtnet_set_affinity(struct virtnet_info *vi)
>>>>>>>> +{
>>>>>>>> + int i;
>>>>>>>> + int cpu;
>>>>>>>> +
>>>>>>>> + /* In multiqueue mode, when the number of cpu is equal to the number of
>>>>>>>> + * queue pairs, we let the queue pairs to be private to one cpu by
>>>>>>>> + * setting the affinity hint to eliminate the contention.
>>>>>>>> + */
>>>>>>>> + if (vi->curr_queue_pairs == 1 ||
>>>>>>>> + vi->max_queue_pairs != num_online_cpus()) {
>>>>>>>> + if (vi->affinity_hint_set)
>>>>>>>> + virtnet_clean_affinity(vi, -1);
>>>>>>>> + else
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + i = 0;
>>>>>>>> + for_each_online_cpu(cpu) {
>>>>>>>> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>>>>>>> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>>>>>> + *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>>>>>> + i++;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + vi->affinity_hint_set = true;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static void virtnet_get_ringparam(struct net_device *dev,
>>>>>>>> struct ethtool_ringparam *ring)
>>>>>>>> {
>>>>>>>> @@ -1105,7 +1114,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>>>>>>> netif_set_real_num_rx_queues(dev, queue_pairs);
>>>>>>>>
>>>>>>>> get_online_cpus();
>>>>>>>> - virtnet_set_affinity(vi, true);
>>>>>>>> + virtnet_set_affinity(vi);
>>>>>>>> put_online_cpus();
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -1274,7 +1283,7 @@ static void virtnet_del_vqs(struct virtnet_info *vi)
>>>>>>>> {
>>>>>>>> struct virtio_device *vdev = vi->vdev;
>>>>>>>>
>>>>>>>> - virtnet_set_affinity(vi, false);
>>>>>>>> + virtnet_clean_affinity(vi, -1);
>>>>>>>>
>>>>>>>> vdev->config->del_vqs(vdev);
>>>>>>>>
>>>>>>>> @@ -1398,7 +1407,7 @@ static int init_vqs(struct virtnet_info *vi)
>>>>>>>> goto err_free;
>>>>>>>>
>>>>>>>> get_online_cpus();
>>>>>>>> - virtnet_set_affinity(vi, true);
>>>>>>>> + virtnet_set_affinity(vi);
>>>>>>>> put_online_cpus();
>>>>>>>>
>>>>>>>> return 0;
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply
* Re: [patch net-next V2] bond: have random dev address by default instead of zeroes
From: Jiri Pirko @ 2013-01-25 7:22 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, davem, andy, stephen, psimerda, dcbw
In-Reply-To: <20130125063713.GA1659@minipsycho.orion>
Fri, Jan 25, 2013 at 07:37:13AM CET, jiri@resnulli.us wrote:
>Fri, Jan 25, 2013 at 02:35:01AM CET, fubar@us.ibm.com wrote:
>>Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>>Makes more sense to have randomly generated address by default than to
>>>have all zeroes. It also allows user to for example put the bond into
>>>bridge without need to have any slaves in it.
>>>
>>>Also note that this changes only behaviour of bonds with no slaves. Once
>>>the first slave device is enslaved, its address will be used (no change
>>>here).
Also, this is not entirely true now. Example:
eth1 has 52:54:00:b8:30:0b
# ip link add bondx address 22:33:44:55:66:77 type bond
# ip link set eth1 master bondx
Now both bondx and eth1 has 22:33:44:55:66:77
Shouldn't both have 52:54:00:b8:30:0b ?
^ permalink raw reply
* Re: [PATCH] tcp: Increment of LINUX_MIB_LISTENOVERFLOWS in tcp_v4_conn_request()
From: Vijay Subramanian @ 2013-01-25 7:29 UTC (permalink / raw)
To: Nivedita Singhvi
Cc: netdev, David Miller, Leandro Lucarella, Nivedita Singhvi
In-Reply-To: <1359097408-9786-1-git-send-email-niv@us.ibm.com>
On 24 January 2013 23:03, Nivedita Singhvi <niveditasinghvi@gmail.com> wrote:
> We drop a connection request if the accept backlog, syn queue are full when processing reaches tcp_v4_conn_request(). Increment the appropriate counter so this isn't silent, for accurate stats and help in debugging.
>
Commit message seems misleading. syn queue is not really full at this
point. It just has more than enough entries.
Thanks,
Vijay
^ permalink raw reply
* Re: [PATCH] tcp: Increment of LINUX_MIB_LISTENOVERFLOWS in tcp_v4_conn_request()
From: Nivedita Singhvi @ 2013-01-25 7:41 UTC (permalink / raw)
To: Vijay Subramanian
Cc: netdev, David Miller, Leandro Lucarella, Nivedita Singhvi
In-Reply-To: <CAGK4HS8ABKqa8J1Sd1Q52P4p3=KcSokQQX0e90FWV6aZsVdP3A@mail.gmail.com>
On 01/24/2013 11:29 PM, Vijay Subramanian wrote:
> On 24 January 2013 23:03, Nivedita Singhvi <niveditasinghvi@gmail.com> wrote:
>> We drop a connection request if the accept backlog, syn queue are full when processing reaches tcp_v4_conn_request(). Increment the appropriate counter so this isn't silent, for accurate stats and help in debugging.
>>
>
> Commit message seems misleading. syn queue is not really full at this
> point. It just has more than enough entries.
Er, good point. I should have said "sufficiently full" or "warm
enough to warrant starting drops" - hope I'm not confusing
things further, am I? DaveM, would you like me to resend, if you're
picking this up?
thanks,
Nivedita
^ permalink raw reply
* Re: [PATCH 2/5] soreuseport: TCP/IPv4 implementation
From: Steffen Klassert @ 2013-01-25 8:08 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, davem, netdev, eric.dumazet
In-Reply-To: <alpine.DEB.2.00.1301221141310.26417@pokey.mtv.corp.google.com>
On Tue, Jan 22, 2013 at 11:50:24AM -0800, Tom Herbert wrote:
> - } else if (tb->fastreuse &&
> - (!sk->sk_reuse || sk->sk_state == TCP_LISTEN))
> - tb->fastreuse = 0;
> + if (sk->sk_reuseport) {
> + tb->fastreuseport = 1;
> + tb->fastuid = uid;
> + } else {
> + tb->fastreuseport = 0;
> + tb->fastuid = 0;
> + }
> + } else {
> + if (tb->fastreuse &&
> + (!sk->sk_reuse || sk->sk_state == TCP_LISTEN))
> + tb->fastreuse = 0;
> + if (tb->fastreuseport &&
> + (!sk->sk_reuseport || !uid_eq(tb->fastuid, uid))) {
> + tb->fastreuseport = 0;
> + tb->fastuid = 0;
> + }
I'm getting the following compile error due to the 0 assignment
to tb->fastuid:
net/ipv4/inet_connection_sock.c: In function ‘inet_csk_get_port’:
net/ipv4/inet_connection_sock.c:232:16: error: incompatible types when assigning to type ‘kuid_t’ from type ‘int’
net/ipv4/inet_connection_sock.c:241:16: error: incompatible types when assigning to type ‘kuid_t’ from type ‘int’
CC lib/show_mem.o
CC net/ipv6/ipv6_sockglue.o
make[3]: *** [net/ipv4/inet_connection_sock.o] Error 1
make[2]: *** [net/ipv4] Error 2
I have not seen this reported so far what surprises me a bit.
This is net-next from today complied with
gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
^ permalink raw reply
* Re: [PATCH net] net: usbnet: prevent buggy devices from killing us
From: Bjørn Mork @ 2013-01-25 8:14 UTC (permalink / raw)
To: Joe Perches
Cc: Oliver Neukum, linux-usb-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1359071829.2117.6.camel@joe-AO722>
Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> writes:
> On Thu, 2013-01-24 at 20:16 +0100, Bjørn Mork wrote:
>> A device sending 0 length frames as fast as it can has been
>> observed killing the host system due to the resulting memory
>> pressure.
> []
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> []
>> @@ -539,6 +545,22 @@ block:
>> break;
>> }
>>
>> + /* stop rx if packet error rate is high */
>> + if (++dev->pkt_cnt > 30) {
>> + dev->pkt_cnt = 0;
>> + dev->pkt_err = 0;
>> + } else {
>> + if (state == rx_cleanup)
>> + dev->pkt_err++;
>> + if (dev->pkt_err > 20) {
>> + set_bit(EVENT_RX_KILL, &dev->flags);
>> + if (net_ratelimit())
>> + netif_dbg(dev, rx_err, dev->net,
>> + "rx kill: high error rate\n");
>> + dev->pkt_err = 0;
>> + }
>> + }
>
> Maybe use ratelimit() here?
>
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> []
>> @@ -33,6 +33,7 @@ struct usbnet {
>> wait_queue_head_t *wait;
>> struct mutex phy_mutex;
>> unsigned char suspend_count;
>> + unsigned char pkt_cnt, pkt_err;
>
> and instead:
>
> struct ratelimit_state errors;
Thanks. I took a look at this, but it seems to be more complex than I
really wanted for keeping the debug noise down here. The rest of usbnet
does not care much about rate limiting debug messages at all. I'll get
a message for every 0 length packet for example.
Maybe usbnet should get a private debug ratelimiter all over?
Is the problem that these instances will hide more important net
messages? Would it help to make the ratelimit call depend on whether
debugging is enabled like ath5k and brcm80211 seems to do?
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: BUG: unable to handle kernel paging request at 0000000000609920 in networking code on 3.2.23.
From: Florian Westphal @ 2013-01-25 8:28 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Florian Westphal, Rafal Kupka, netdev, netfilter-devel
In-Reply-To: <alpine.LNX.2.01.1301250124310.21495@nerf07.vanv.qr>
Jan Engelhardt <jengelh@inai.de> wrote:
> On Friday 2013-01-25 00:08, Florian Westphal wrote:
> >@@ -35,10 +35,18 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
> > {
> > unsigned int optl, i, j;
> > struct tcphdr *tcph;
> >+ struct tcphdr _tcph;
> > u_int16_t n, o;
> > u_int8_t *opt;
> >
> >- if (!skb_make_writable(skb, skb->len))
> >+ if (skb->len < minlen)
> >+ return XT_CONTINUE;
> >+
> >+ tcph = skb_header_pointer(skb, tcphoff, sizeof(_tcph), &_tcph);
> >+ if (!tcph)
> >+ return XT_CONTINUE; /* no options -> nothing to do */
>
> To the best of my analysis, the "no options" comment is incorrect here,
> because you are not even looking at the options so far, but only tcph.
Yup.
> The prose should probably be something like:
> if (iph->frag_off & htons(IP_OFFSET)) != 0)
> /* not the first fragment - lost case */
> return XT_CONTINUE;
[..]
Can to submit a patch?
^ permalink raw reply
* [PATCH V7 1/3] virtio-net: fix the set affinity bug when CPU IDs are not consecutive
From: Wanlong Gao @ 2013-01-25 8:36 UTC (permalink / raw)
To: linux-kernel
Cc: Michael S. Tsirkin, netdev, virtualization, Eric Dumazet,
David S. Miller
As Michael mentioned, set affinity and select queue will not work very
well when CPU IDs are not consecutive, this can happen with hot unplug.
Fix this bug by traversal the online CPUs, and create a per cpu variable
to find the mapping from CPU to the preferable virtual-queue.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Eric Dumazet <erdnetdev@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
V6->V7:
serialize virtnet_set_queues to avoid a race with cpu hotplug (Jason)
V5->V6:
remove {get|put}_online_cpus from virtnet_del_vqs (Jason)
V4->V5:
Add get/put_online_cpus to avoid CPUs go up and down during operations (Rusty)
V3->V4:
move vq_index into virtnet_info (Jason)
change the mapping value when not setting affinity (Jason)
address the comments about select_queue (Rusty)
drivers/net/virtio_net.c | 58 +++++++++++++++++++++++++++++++++++++++---------
1 file changed, 47 insertions(+), 11 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a6fcf15..0f3afa8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -123,6 +123,9 @@ struct virtnet_info {
/* Does the affinity hint is set for virtqueues? */
bool affinity_hint_set;
+
+ /* Per-cpu variable to show the mapping from CPU to virtqueue */
+ int __percpu *vq_index;
};
struct skb_vnet_hdr {
@@ -1016,6 +1019,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
{
int i;
+ int cpu;
/* In multiqueue mode, when the number of cpu is equal to the number of
* queue pairs, we let the queue pairs to be private to one cpu by
@@ -1029,16 +1033,29 @@ static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
return;
}
- for (i = 0; i < vi->max_queue_pairs; i++) {
- int cpu = set ? i : -1;
- virtqueue_set_affinity(vi->rq[i].vq, cpu);
- virtqueue_set_affinity(vi->sq[i].vq, cpu);
- }
+ if (set) {
+ i = 0;
+ for_each_online_cpu(cpu) {
+ virtqueue_set_affinity(vi->rq[i].vq, cpu);
+ virtqueue_set_affinity(vi->sq[i].vq, cpu);
+ *per_cpu_ptr(vi->vq_index, cpu) = i;
+ i++;
+ }
- if (set)
vi->affinity_hint_set = true;
- else
+ } else {
+ for(i = 0; i < vi->max_queue_pairs; i++) {
+ virtqueue_set_affinity(vi->rq[i].vq, -1);
+ virtqueue_set_affinity(vi->sq[i].vq, -1);
+ }
+
+ i = 0;
+ for_each_online_cpu(cpu)
+ *per_cpu_ptr(vi->vq_index, cpu) =
+ ++i % vi->curr_queue_pairs;
+
vi->affinity_hint_set = false;
+ }
}
static void virtnet_get_ringparam(struct net_device *dev,
@@ -1082,6 +1099,7 @@ static int virtnet_set_channels(struct net_device *dev,
if (queue_pairs > vi->max_queue_pairs)
return -EINVAL;
+ get_online_cpus();
err = virtnet_set_queues(vi, queue_pairs);
if (!err) {
netif_set_real_num_tx_queues(dev, queue_pairs);
@@ -1089,6 +1107,7 @@ static int virtnet_set_channels(struct net_device *dev,
virtnet_set_affinity(vi, true);
}
+ put_online_cpus();
return err;
}
@@ -1127,12 +1146,19 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
/* To avoid contending a lock hold by a vcpu who would exit to host, select the
* txq based on the processor id.
- * TODO: handle cpu hotplug.
*/
static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
{
- int txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) :
- smp_processor_id();
+ int txq;
+ struct virtnet_info *vi = netdev_priv(dev);
+
+ if (skb_rx_queue_recorded(skb)) {
+ txq = skb_get_rx_queue(skb);
+ } else {
+ txq = *__this_cpu_ptr(vi->vq_index);
+ if (txq == -1)
+ txq = 0;
+ }
while (unlikely(txq >= dev->real_num_tx_queues))
txq -= dev->real_num_tx_queues;
@@ -1371,7 +1397,10 @@ static int init_vqs(struct virtnet_info *vi)
if (ret)
goto err_free;
+ get_online_cpus();
virtnet_set_affinity(vi, true);
+ put_online_cpus();
+
return 0;
err_free:
@@ -1453,6 +1482,10 @@ static int virtnet_probe(struct virtio_device *vdev)
if (vi->stats == NULL)
goto free;
+ vi->vq_index = alloc_percpu(int);
+ if (vi->vq_index == NULL)
+ goto free_stats;
+
mutex_init(&vi->config_lock);
vi->config_enable = true;
INIT_WORK(&vi->config_work, virtnet_config_changed_work);
@@ -1476,7 +1509,7 @@ static int virtnet_probe(struct virtio_device *vdev)
/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
err = init_vqs(vi);
if (err)
- goto free_stats;
+ goto free_index;
netif_set_real_num_tx_queues(dev, 1);
netif_set_real_num_rx_queues(dev, 1);
@@ -1520,6 +1553,8 @@ free_recv_bufs:
free_vqs:
cancel_delayed_work_sync(&vi->refill);
virtnet_del_vqs(vi);
+free_index:
+ free_percpu(vi->vq_index);
free_stats:
free_percpu(vi->stats);
free:
@@ -1554,6 +1589,7 @@ static void virtnet_remove(struct virtio_device *vdev)
flush_work(&vi->config_work);
+ free_percpu(vi->vq_index);
free_percpu(vi->stats);
free_netdev(vi->dev);
}
--
1.8.1
^ permalink raw reply related
* [PATCH V7 2/3] virtio-net: split out clean affinity function
From: Wanlong Gao @ 2013-01-25 8:36 UTC (permalink / raw)
To: linux-kernel
Cc: Michael S. Tsirkin, netdev, virtualization, Eric Dumazet,
David S. Miller
In-Reply-To: <1359102981-3401-1-git-send-email-gaowanlong@cn.fujitsu.com>
Split out the clean affinity function to virtnet_clean_affinity().
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Eric Dumazet <erdnetdev@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
V6->V7:
always set vq_index (Jason)
V5->V6: NEW
drivers/net/virtio_net.c | 71 +++++++++++++++++++++++++++---------------------
1 file changed, 40 insertions(+), 31 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0f3afa8..2f6fe9b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1016,46 +1016,55 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
return 0;
}
-static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
+static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
{
int i;
int cpu;
- /* In multiqueue mode, when the number of cpu is equal to the number of
- * queue pairs, we let the queue pairs to be private to one cpu by
- * setting the affinity hint to eliminate the contention.
- */
- if ((vi->curr_queue_pairs == 1 ||
- vi->max_queue_pairs != num_online_cpus()) && set) {
- if (vi->affinity_hint_set)
- set = false;
- else
- return;
- }
-
- if (set) {
- i = 0;
- for_each_online_cpu(cpu) {
- virtqueue_set_affinity(vi->rq[i].vq, cpu);
- virtqueue_set_affinity(vi->sq[i].vq, cpu);
- *per_cpu_ptr(vi->vq_index, cpu) = i;
- i++;
- }
-
- vi->affinity_hint_set = true;
- } else {
- for(i = 0; i < vi->max_queue_pairs; i++) {
+ if (vi->affinity_hint_set) {
+ for (i = 0; i < vi->max_queue_pairs; i++) {
virtqueue_set_affinity(vi->rq[i].vq, -1);
virtqueue_set_affinity(vi->sq[i].vq, -1);
}
- i = 0;
- for_each_online_cpu(cpu)
+ vi->affinity_hint_set = false;
+ }
+
+ i = 0;
+ for_each_online_cpu(cpu) {
+ if (cpu == hcpu) {
+ *per_cpu_ptr(vi->vq_index, cpu) = -1;
+ } else {
*per_cpu_ptr(vi->vq_index, cpu) =
++i % vi->curr_queue_pairs;
+ }
+ }
+}
- vi->affinity_hint_set = false;
+static void virtnet_set_affinity(struct virtnet_info *vi)
+{
+ int i;
+ int cpu;
+
+ /* In multiqueue mode, when the number of cpu is equal to the number of
+ * queue pairs, we let the queue pairs to be private to one cpu by
+ * setting the affinity hint to eliminate the contention.
+ */
+ if (vi->curr_queue_pairs == 1 ||
+ vi->max_queue_pairs != num_online_cpus()) {
+ virtnet_clean_affinity(vi, -1);
+ return;
}
+
+ i = 0;
+ for_each_online_cpu(cpu) {
+ virtqueue_set_affinity(vi->rq[i].vq, cpu);
+ virtqueue_set_affinity(vi->sq[i].vq, cpu);
+ *per_cpu_ptr(vi->vq_index, cpu) = i;
+ i++;
+ }
+
+ vi->affinity_hint_set = true;
}
static void virtnet_get_ringparam(struct net_device *dev,
@@ -1105,7 +1114,7 @@ static int virtnet_set_channels(struct net_device *dev,
netif_set_real_num_tx_queues(dev, queue_pairs);
netif_set_real_num_rx_queues(dev, queue_pairs);
- virtnet_set_affinity(vi, true);
+ virtnet_set_affinity(vi);
}
put_online_cpus();
@@ -1274,7 +1283,7 @@ static void virtnet_del_vqs(struct virtnet_info *vi)
{
struct virtio_device *vdev = vi->vdev;
- virtnet_set_affinity(vi, false);
+ virtnet_clean_affinity(vi, -1);
vdev->config->del_vqs(vdev);
@@ -1398,7 +1407,7 @@ static int init_vqs(struct virtnet_info *vi)
goto err_free;
get_online_cpus();
- virtnet_set_affinity(vi, true);
+ virtnet_set_affinity(vi);
put_online_cpus();
return 0;
--
1.8.1
^ permalink raw reply related
* [PATCH V7 3/3] virtio-net: reset virtqueue affinity when doing cpu hotplug
From: Wanlong Gao @ 2013-01-25 8:36 UTC (permalink / raw)
To: linux-kernel
Cc: Michael S. Tsirkin, netdev, virtualization, Eric Dumazet,
David S. Miller
In-Reply-To: <1359102981-3401-1-git-send-email-gaowanlong@cn.fujitsu.com>
Add a cpu notifier to virtio-net, so that we can reset the
virtqueue affinity if the cpu hotplug happens. It improve
the performance through enabling or disabling the virtqueue
affinity after doing cpu hotplug.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Eric Dumazet <erdnetdev@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
V6->V7: no change
V5->V6:
deal with CPU_DOWN_PREPARE separately by just cleaning affinity
V4->V5:
New method to deal with the cpu hotplug actions (Rusty)
drivers/net/virtio_net.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2f6fe9b..35c00c5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -26,6 +26,7 @@
#include <linux/scatterlist.h>
#include <linux/if_vlan.h>
#include <linux/slab.h>
+#include <linux/cpu.h>
static int napi_weight = 128;
module_param(napi_weight, int, 0444);
@@ -126,6 +127,9 @@ struct virtnet_info {
/* Per-cpu variable to show the mapping from CPU to virtqueue */
int __percpu *vq_index;
+
+ /* CPU hot plug notifier */
+ struct notifier_block nb;
};
struct skb_vnet_hdr {
@@ -1067,6 +1071,26 @@ static void virtnet_set_affinity(struct virtnet_info *vi)
vi->affinity_hint_set = true;
}
+static int virtnet_cpu_callback(struct notifier_block *nfb,
+ unsigned long action, void *hcpu)
+{
+ struct virtnet_info *vi = container_of(nfb, struct virtnet_info, nb);
+
+ switch(action & ~CPU_TASKS_FROZEN) {
+ case CPU_ONLINE:
+ case CPU_DOWN_FAILED:
+ case CPU_DEAD:
+ virtnet_set_affinity(vi);
+ break;
+ case CPU_DOWN_PREPARE:
+ virtnet_clean_affinity(vi, (long)hcpu);
+ break;
+ default:
+ break;
+ }
+ return NOTIFY_OK;
+}
+
static void virtnet_get_ringparam(struct net_device *dev,
struct ethtool_ringparam *ring)
{
@@ -1541,6 +1565,13 @@ static int virtnet_probe(struct virtio_device *vdev)
}
}
+ vi->nb.notifier_call = &virtnet_cpu_callback;
+ err = register_hotcpu_notifier(&vi->nb);
+ if (err) {
+ pr_debug("virtio_net: registering cpu notifier failed\n");
+ goto free_recv_bufs;
+ }
+
/* Assume link up if device can't report link status,
otherwise get link status from config. */
if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
@@ -1587,6 +1618,8 @@ static void virtnet_remove(struct virtio_device *vdev)
{
struct virtnet_info *vi = vdev->priv;
+ unregister_hotcpu_notifier(&vi->nb);
+
/* Prevent config work handler from accessing the device. */
mutex_lock(&vi->config_lock);
vi->config_enable = false;
--
1.8.1
^ permalink raw reply related
* Re: [BUG] Bug in netprio_cgroup and netcls_cgroup ?
From: Li Zefan @ 2013-01-25 8:39 UTC (permalink / raw)
To: Daniel Wagner
Cc: John Fastabend, John Fastabend, Neil Horman, Daniel Wagner, LKML,
netdev, Cgroups
In-Reply-To: <20130122100938.GA26820@candlejack.bmw-carit.intra>
>>>> BTW, isn't this a similar to what should happen with the block io cgroup?
>>>> What is the behavior with a fd writing to a file in the scenario you
>>>> describe above?
>>>>
>>>
>>> It forbids task moving in this case:
>>>
>>> /*
>>> * We cannot support shared io contexts, as we have no mean to support
>>> * two tasks with the same ioc in two different groups without major rework
>>> * of the main cic data structures. For now we allow a task to change
>>> * its cgroup only if it's the only owner of its ioc.
>>> */
>>> static int blkcg_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
>>>
>>
>> OK, I guess we should do something similar in the netprio, netcls
>> cgroups and
>> yes document it as you noted in your last comment.
>
> Here is my attempt to add such a check. I really don't know if this is the
> correct way to do so. To test this I have written a test program, which
> seems to test the right thing. Please have a look and let me know if
> it is correct: http://www.monom.org/misc/scm_rights.c
>
> And here a dirty first version of the patch:
>
Adding new can_attach() is discouraged. It's considered bad, because if cgroup
is mounted with multi subsystems binded, and if one of them can forbid task
moving for some reason, this will add burden on users to use cgroup properly.
^ permalink raw reply
* Re: [PATCH V7 1/3] virtio-net: fix the set affinity bug when CPU IDs are not consecutive
From: Jason Wang @ 2013-01-25 9:00 UTC (permalink / raw)
To: Wanlong Gao
Cc: Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Eric Dumazet, David S. Miller
In-Reply-To: <1359102981-3401-1-git-send-email-gaowanlong@cn.fujitsu.com>
On 01/25/2013 04:36 PM, Wanlong Gao wrote:
> As Michael mentioned, set affinity and select queue will not work very
> well when CPU IDs are not consecutive, this can happen with hot unplug.
> Fix this bug by traversal the online CPUs, and create a per cpu variable
> to find the mapping from CPU to the preferable virtual-queue.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Eric Dumazet <erdnetdev@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: virtualization@lists.linux-foundation.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> V6->V7:
> serialize virtnet_set_queues to avoid a race with cpu hotplug (Jason)
> V5->V6:
> remove {get|put}_online_cpus from virtnet_del_vqs (Jason)
> V4->V5:
> Add get/put_online_cpus to avoid CPUs go up and down during operations (Rusty)
>
> V3->V4:
> move vq_index into virtnet_info (Jason)
> change the mapping value when not setting affinity (Jason)
> address the comments about select_queue (Rusty)
>
> drivers/net/virtio_net.c | 58 +++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a6fcf15..0f3afa8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -123,6 +123,9 @@ struct virtnet_info {
>
> /* Does the affinity hint is set for virtqueues? */
> bool affinity_hint_set;
> +
> + /* Per-cpu variable to show the mapping from CPU to virtqueue */
> + int __percpu *vq_index;
> };
>
> struct skb_vnet_hdr {
> @@ -1016,6 +1019,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
> static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
> {
> int i;
> + int cpu;
>
> /* In multiqueue mode, when the number of cpu is equal to the number of
> * queue pairs, we let the queue pairs to be private to one cpu by
> @@ -1029,16 +1033,29 @@ static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
> return;
> }
>
> - for (i = 0; i < vi->max_queue_pairs; i++) {
> - int cpu = set ? i : -1;
> - virtqueue_set_affinity(vi->rq[i].vq, cpu);
> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
> - }
> + if (set) {
> + i = 0;
> + for_each_online_cpu(cpu) {
> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
> + *per_cpu_ptr(vi->vq_index, cpu) = i;
> + i++;
> + }
>
> - if (set)
> vi->affinity_hint_set = true;
> - else
> + } else {
> + for(i = 0; i < vi->max_queue_pairs; i++) {
> + virtqueue_set_affinity(vi->rq[i].vq, -1);
> + virtqueue_set_affinity(vi->sq[i].vq, -1);
> + }
> +
> + i = 0;
> + for_each_online_cpu(cpu)
> + *per_cpu_ptr(vi->vq_index, cpu) =
> + ++i % vi->curr_queue_pairs;
> +
> vi->affinity_hint_set = false;
> + }
> }
Sorry, looks like the issue of v6 still exists, we need set per-cpu
index unconditionally here (and also in 2/3), the cpus != queues check
may bypass this setting.
>
> static void virtnet_get_ringparam(struct net_device *dev,
> @@ -1082,6 +1099,7 @@ static int virtnet_set_channels(struct net_device *dev,
> if (queue_pairs > vi->max_queue_pairs)
> return -EINVAL;
>
> + get_online_cpus();
> err = virtnet_set_queues(vi, queue_pairs);
> if (!err) {
> netif_set_real_num_tx_queues(dev, queue_pairs);
> @@ -1089,6 +1107,7 @@ static int virtnet_set_channels(struct net_device *dev,
>
> virtnet_set_affinity(vi, true);
> }
> + put_online_cpus();
>
> return err;
> }
> @@ -1127,12 +1146,19 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
>
> /* To avoid contending a lock hold by a vcpu who would exit to host, select the
> * txq based on the processor id.
> - * TODO: handle cpu hotplug.
> */
> static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
> {
> - int txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) :
> - smp_processor_id();
> + int txq;
> + struct virtnet_info *vi = netdev_priv(dev);
> +
> + if (skb_rx_queue_recorded(skb)) {
> + txq = skb_get_rx_queue(skb);
> + } else {
> + txq = *__this_cpu_ptr(vi->vq_index);
> + if (txq == -1)
> + txq = 0;
> + }
>
> while (unlikely(txq >= dev->real_num_tx_queues))
> txq -= dev->real_num_tx_queues;
> @@ -1371,7 +1397,10 @@ static int init_vqs(struct virtnet_info *vi)
> if (ret)
> goto err_free;
>
> + get_online_cpus();
> virtnet_set_affinity(vi, true);
> + put_online_cpus();
> +
> return 0;
>
> err_free:
> @@ -1453,6 +1482,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (vi->stats == NULL)
> goto free;
>
> + vi->vq_index = alloc_percpu(int);
> + if (vi->vq_index == NULL)
> + goto free_stats;
> +
> mutex_init(&vi->config_lock);
> vi->config_enable = true;
> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
> @@ -1476,7 +1509,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> - goto free_stats;
> + goto free_index;
>
> netif_set_real_num_tx_queues(dev, 1);
> netif_set_real_num_rx_queues(dev, 1);
> @@ -1520,6 +1553,8 @@ free_recv_bufs:
> free_vqs:
> cancel_delayed_work_sync(&vi->refill);
> virtnet_del_vqs(vi);
> +free_index:
> + free_percpu(vi->vq_index);
> free_stats:
> free_percpu(vi->stats);
> free:
> @@ -1554,6 +1589,7 @@ static void virtnet_remove(struct virtio_device *vdev)
>
> flush_work(&vi->config_work);
>
> + free_percpu(vi->vq_index);
> free_percpu(vi->stats);
> free_netdev(vi->dev);
> }
^ permalink raw reply
* Re: [PATCH V7 1/3] virtio-net: fix the set affinity bug when CPU IDs are not consecutive
From: Wanlong Gao @ 2013-01-25 9:05 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Eric Dumazet, David S. Miller
In-Reply-To: <510249C6.2000105@redhat.com>
On 01/25/2013 05:00 PM, Jason Wang wrote:
> On 01/25/2013 04:36 PM, Wanlong Gao wrote:
>> As Michael mentioned, set affinity and select queue will not work very
>> well when CPU IDs are not consecutive, this can happen with hot unplug.
>> Fix this bug by traversal the online CPUs, and create a per cpu variable
>> to find the mapping from CPU to the preferable virtual-queue.
>>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Eric Dumazet <erdnetdev@gmail.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> V6->V7:
>> serialize virtnet_set_queues to avoid a race with cpu hotplug (Jason)
>> V5->V6:
>> remove {get|put}_online_cpus from virtnet_del_vqs (Jason)
>> V4->V5:
>> Add get/put_online_cpus to avoid CPUs go up and down during operations (Rusty)
>>
>> V3->V4:
>> move vq_index into virtnet_info (Jason)
>> change the mapping value when not setting affinity (Jason)
>> address the comments about select_queue (Rusty)
>>
>> drivers/net/virtio_net.c | 58 +++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 47 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index a6fcf15..0f3afa8 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -123,6 +123,9 @@ struct virtnet_info {
>>
>> /* Does the affinity hint is set for virtqueues? */
>> bool affinity_hint_set;
>> +
>> + /* Per-cpu variable to show the mapping from CPU to virtqueue */
>> + int __percpu *vq_index;
>> };
>>
>> struct skb_vnet_hdr {
>> @@ -1016,6 +1019,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>> static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>> {
>> int i;
>> + int cpu;
>>
>> /* In multiqueue mode, when the number of cpu is equal to the number of
>> * queue pairs, we let the queue pairs to be private to one cpu by
>> @@ -1029,16 +1033,29 @@ static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>> return;
>> }
>>
>> - for (i = 0; i < vi->max_queue_pairs; i++) {
>> - int cpu = set ? i : -1;
>> - virtqueue_set_affinity(vi->rq[i].vq, cpu);
>> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
>> - }
>> + if (set) {
>> + i = 0;
>> + for_each_online_cpu(cpu) {
>> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
>> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
>> + *per_cpu_ptr(vi->vq_index, cpu) = i;
>> + i++;
>> + }
>>
>> - if (set)
>> vi->affinity_hint_set = true;
>> - else
>> + } else {
>> + for(i = 0; i < vi->max_queue_pairs; i++) {
>> + virtqueue_set_affinity(vi->rq[i].vq, -1);
>> + virtqueue_set_affinity(vi->sq[i].vq, -1);
>> + }
>> +
>> + i = 0;
>> + for_each_online_cpu(cpu)
>> + *per_cpu_ptr(vi->vq_index, cpu) =
>> + ++i % vi->curr_queue_pairs;
>> +
>> vi->affinity_hint_set = false;
>> + }
>> }
>
> Sorry, looks like the issue of v6 still exists, we need set per-cpu
> index unconditionally here (and also in 2/3), the cpus != queues check
> may bypass this setting.
This fixed in 2/3, when cpus != queues, it will go into virtnet_clean_affinity(in 2/3),
then vq index is set in virtnet_clean_affinity. Am I missing something?
Thanks,
Wanlong Gao
>>
>> static void virtnet_get_ringparam(struct net_device *dev,
>> @@ -1082,6 +1099,7 @@ static int virtnet_set_channels(struct net_device *dev,
>> if (queue_pairs > vi->max_queue_pairs)
>> return -EINVAL;
>>
>> + get_online_cpus();
>> err = virtnet_set_queues(vi, queue_pairs);
>> if (!err) {
>> netif_set_real_num_tx_queues(dev, queue_pairs);
>> @@ -1089,6 +1107,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>
>> virtnet_set_affinity(vi, true);
>> }
>> + put_online_cpus();
>>
>> return err;
>> }
>> @@ -1127,12 +1146,19 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
>>
>> /* To avoid contending a lock hold by a vcpu who would exit to host, select the
>> * txq based on the processor id.
>> - * TODO: handle cpu hotplug.
>> */
>> static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
>> {
>> - int txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) :
>> - smp_processor_id();
>> + int txq;
>> + struct virtnet_info *vi = netdev_priv(dev);
>> +
>> + if (skb_rx_queue_recorded(skb)) {
>> + txq = skb_get_rx_queue(skb);
>> + } else {
>> + txq = *__this_cpu_ptr(vi->vq_index);
>> + if (txq == -1)
>> + txq = 0;
>> + }
>>
>> while (unlikely(txq >= dev->real_num_tx_queues))
>> txq -= dev->real_num_tx_queues;
>> @@ -1371,7 +1397,10 @@ static int init_vqs(struct virtnet_info *vi)
>> if (ret)
>> goto err_free;
>>
>> + get_online_cpus();
>> virtnet_set_affinity(vi, true);
>> + put_online_cpus();
>> +
>> return 0;
>>
>> err_free:
>> @@ -1453,6 +1482,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>> if (vi->stats == NULL)
>> goto free;
>>
>> + vi->vq_index = alloc_percpu(int);
>> + if (vi->vq_index == NULL)
>> + goto free_stats;
>> +
>> mutex_init(&vi->config_lock);
>> vi->config_enable = true;
>> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
>> @@ -1476,7 +1509,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
>> err = init_vqs(vi);
>> if (err)
>> - goto free_stats;
>> + goto free_index;
>>
>> netif_set_real_num_tx_queues(dev, 1);
>> netif_set_real_num_rx_queues(dev, 1);
>> @@ -1520,6 +1553,8 @@ free_recv_bufs:
>> free_vqs:
>> cancel_delayed_work_sync(&vi->refill);
>> virtnet_del_vqs(vi);
>> +free_index:
>> + free_percpu(vi->vq_index);
>> free_stats:
>> free_percpu(vi->stats);
>> free:
>> @@ -1554,6 +1589,7 @@ static void virtnet_remove(struct virtio_device *vdev)
>>
>> flush_work(&vi->config_work);
>>
>> + free_percpu(vi->vq_index);
>> free_percpu(vi->stats);
>> free_netdev(vi->dev);
>> }
>
>
^ permalink raw reply
* Re: [PATCH V7 1/3] virtio-net: fix the set affinity bug when CPU IDs are not consecutive
From: Jason Wang @ 2013-01-25 9:11 UTC (permalink / raw)
To: gaowanlong
Cc: Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Eric Dumazet, David S. Miller
In-Reply-To: <51024AC7.9030109@cn.fujitsu.com>
On 01/25/2013 05:05 PM, Wanlong Gao wrote:
> On 01/25/2013 05:00 PM, Jason Wang wrote:
>> On 01/25/2013 04:36 PM, Wanlong Gao wrote:
>>> As Michael mentioned, set affinity and select queue will not work very
>>> well when CPU IDs are not consecutive, this can happen with hot unplug.
>>> Fix this bug by traversal the online CPUs, and create a per cpu variable
>>> to find the mapping from CPU to the preferable virtual-queue.
>>>
>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: Eric Dumazet <erdnetdev@gmail.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: virtualization@lists.linux-foundation.org
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>> V6->V7:
>>> serialize virtnet_set_queues to avoid a race with cpu hotplug (Jason)
>>> V5->V6:
>>> remove {get|put}_online_cpus from virtnet_del_vqs (Jason)
>>> V4->V5:
>>> Add get/put_online_cpus to avoid CPUs go up and down during operations (Rusty)
>>>
>>> V3->V4:
>>> move vq_index into virtnet_info (Jason)
>>> change the mapping value when not setting affinity (Jason)
>>> address the comments about select_queue (Rusty)
>>>
>>> drivers/net/virtio_net.c | 58 +++++++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 47 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index a6fcf15..0f3afa8 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -123,6 +123,9 @@ struct virtnet_info {
>>>
>>> /* Does the affinity hint is set for virtqueues? */
>>> bool affinity_hint_set;
>>> +
>>> + /* Per-cpu variable to show the mapping from CPU to virtqueue */
>>> + int __percpu *vq_index;
>>> };
>>>
>>> struct skb_vnet_hdr {
>>> @@ -1016,6 +1019,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>>> static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>>> {
>>> int i;
>>> + int cpu;
>>>
>>> /* In multiqueue mode, when the number of cpu is equal to the number of
>>> * queue pairs, we let the queue pairs to be private to one cpu by
>>> @@ -1029,16 +1033,29 @@ static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>>> return;
>>> }
>>>
>>> - for (i = 0; i < vi->max_queue_pairs; i++) {
>>> - int cpu = set ? i : -1;
>>> - virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>> - }
>>> + if (set) {
>>> + i = 0;
>>> + for_each_online_cpu(cpu) {
>>> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>> + *per_cpu_ptr(vi->vq_index, cpu) = i;
>>> + i++;
>>> + }
>>>
>>> - if (set)
>>> vi->affinity_hint_set = true;
>>> - else
>>> + } else {
>>> + for(i = 0; i < vi->max_queue_pairs; i++) {
>>> + virtqueue_set_affinity(vi->rq[i].vq, -1);
>>> + virtqueue_set_affinity(vi->sq[i].vq, -1);
>>> + }
>>> +
>>> + i = 0;
>>> + for_each_online_cpu(cpu)
>>> + *per_cpu_ptr(vi->vq_index, cpu) =
>>> + ++i % vi->curr_queue_pairs;
>>> +
>>> vi->affinity_hint_set = false;
>>> + }
>>> }
>> Sorry, looks like the issue of v6 still exists, we need set per-cpu
>> index unconditionally here (and also in 2/3), the cpus != queues check
>> may bypass this setting.
> This fixed in 2/3, when cpus != queues, it will go into virtnet_clean_affinity(in 2/3),
> then vq index is set in virtnet_clean_affinity. Am I missing something?
Ah, so 2/3 looks fine. I suggest to fix this in 1/3 since it's not good
to introduce a bug in patch 1 and fix it in patch 2, and this can also
confuse the bisect.
Thanks
> Thanks,
> Wanlong Gao
>
>>>
>>> static void virtnet_get_ringparam(struct net_device *dev,
>>> @@ -1082,6 +1099,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>> if (queue_pairs > vi->max_queue_pairs)
>>> return -EINVAL;
>>>
>>> + get_online_cpus();
>>> err = virtnet_set_queues(vi, queue_pairs);
>>> if (!err) {
>>> netif_set_real_num_tx_queues(dev, queue_pairs);
>>> @@ -1089,6 +1107,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>>
>>> virtnet_set_affinity(vi, true);
>>> }
>>> + put_online_cpus();
>>>
>>> return err;
>>> }
>>> @@ -1127,12 +1146,19 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
>>>
>>> /* To avoid contending a lock hold by a vcpu who would exit to host, select the
>>> * txq based on the processor id.
>>> - * TODO: handle cpu hotplug.
>>> */
>>> static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
>>> {
>>> - int txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) :
>>> - smp_processor_id();
>>> + int txq;
>>> + struct virtnet_info *vi = netdev_priv(dev);
>>> +
>>> + if (skb_rx_queue_recorded(skb)) {
>>> + txq = skb_get_rx_queue(skb);
>>> + } else {
>>> + txq = *__this_cpu_ptr(vi->vq_index);
>>> + if (txq == -1)
>>> + txq = 0;
>>> + }
>>>
>>> while (unlikely(txq >= dev->real_num_tx_queues))
>>> txq -= dev->real_num_tx_queues;
>>> @@ -1371,7 +1397,10 @@ static int init_vqs(struct virtnet_info *vi)
>>> if (ret)
>>> goto err_free;
>>>
>>> + get_online_cpus();
>>> virtnet_set_affinity(vi, true);
>>> + put_online_cpus();
>>> +
>>> return 0;
>>>
>>> err_free:
>>> @@ -1453,6 +1482,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>>> if (vi->stats == NULL)
>>> goto free;
>>>
>>> + vi->vq_index = alloc_percpu(int);
>>> + if (vi->vq_index == NULL)
>>> + goto free_stats;
>>> +
>>> mutex_init(&vi->config_lock);
>>> vi->config_enable = true;
>>> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
>>> @@ -1476,7 +1509,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>>> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
>>> err = init_vqs(vi);
>>> if (err)
>>> - goto free_stats;
>>> + goto free_index;
>>>
>>> netif_set_real_num_tx_queues(dev, 1);
>>> netif_set_real_num_rx_queues(dev, 1);
>>> @@ -1520,6 +1553,8 @@ free_recv_bufs:
>>> free_vqs:
>>> cancel_delayed_work_sync(&vi->refill);
>>> virtnet_del_vqs(vi);
>>> +free_index:
>>> + free_percpu(vi->vq_index);
>>> free_stats:
>>> free_percpu(vi->stats);
>>> free:
>>> @@ -1554,6 +1589,7 @@ static void virtnet_remove(struct virtio_device *vdev)
>>>
>>> flush_work(&vi->config_work);
>>>
>>> + free_percpu(vi->vq_index);
>>> free_percpu(vi->stats);
>>> free_netdev(vi->dev);
>>> }
>>
^ permalink raw reply
* Re: [PATCH V7 1/3] virtio-net: fix the set affinity bug when CPU IDs are not consecutive
From: Wanlong Gao @ 2013-01-25 9:16 UTC (permalink / raw)
To: Jason Wang
Cc: linux-kernel, Rusty Russell, Michael S. Tsirkin, Eric Dumazet,
David S. Miller, virtualization, netdev, Wanlong Gao
In-Reply-To: <51024C3A.6080200@redhat.com>
>>>>
>>>> - for (i = 0; i < vi->max_queue_pairs; i++) {
>>>> - int cpu = set ? i : -1;
>>>> - virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>>> - virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>> - }
>>>> + if (set) {
>>>> + i = 0;
>>>> + for_each_online_cpu(cpu) {
>>>> + virtqueue_set_affinity(vi->rq[i].vq, cpu);
>>>> + virtqueue_set_affinity(vi->sq[i].vq, cpu);
>>>> + *per_cpu_ptr(vi->vq_index, cpu) = i;
>>>> + i++;
>>>> + }
>>>>
>>>> - if (set)
>>>> vi->affinity_hint_set = true;
>>>> - else
>>>> + } else {
>>>> + for(i = 0; i < vi->max_queue_pairs; i++) {
>>>> + virtqueue_set_affinity(vi->rq[i].vq, -1);
>>>> + virtqueue_set_affinity(vi->sq[i].vq, -1);
>>>> + }
>>>> +
>>>> + i = 0;
>>>> + for_each_online_cpu(cpu)
>>>> + *per_cpu_ptr(vi->vq_index, cpu) =
>>>> + ++i % vi->curr_queue_pairs;
>>>> +
>>>> vi->affinity_hint_set = false;
>>>> + }
>>>> }
>>> Sorry, looks like the issue of v6 still exists, we need set per-cpu
>>> index unconditionally here (and also in 2/3), the cpus != queues check
>>> may bypass this setting.
>> This fixed in 2/3, when cpus != queues, it will go into virtnet_clean_affinity(in 2/3),
>> then vq index is set in virtnet_clean_affinity. Am I missing something?
>
> Ah, so 2/3 looks fine. I suggest to fix this in 1/3 since it's not good
> to introduce a bug in patch 1 and fix it in patch 2, and this can also
> confuse the bisect.
>
Make sense, will move the fix from 2/3 to 1/3.
Thanks,
Wanlong Gao
^ permalink raw reply
* [PATCH V8 3/3] virtio-net: reset virtqueue affinity when doing cpu hotplug
From: Wanlong Gao @ 2013-01-25 9:51 UTC (permalink / raw)
To: linux-kernel
Cc: Michael S. Tsirkin, netdev, virtualization, Eric Dumazet,
David S. Miller
In-Reply-To: <1359107491-6749-1-git-send-email-gaowanlong@cn.fujitsu.com>
Add a cpu notifier to virtio-net, so that we can reset the
virtqueue affinity if the cpu hotplug happens. It improve
the performance through enabling or disabling the virtqueue
affinity after doing cpu hotplug.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Eric Dumazet <erdnetdev@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
V5->V6:
deal with CPU_DOWN_PREPARE separately by just cleaning affinity
V4->V5:
New method to deal with the cpu hotplug actions (Rusty)
drivers/net/virtio_net.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2f6fe9b..35c00c5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -26,6 +26,7 @@
#include <linux/scatterlist.h>
#include <linux/if_vlan.h>
#include <linux/slab.h>
+#include <linux/cpu.h>
static int napi_weight = 128;
module_param(napi_weight, int, 0444);
@@ -126,6 +127,9 @@ struct virtnet_info {
/* Per-cpu variable to show the mapping from CPU to virtqueue */
int __percpu *vq_index;
+
+ /* CPU hot plug notifier */
+ struct notifier_block nb;
};
struct skb_vnet_hdr {
@@ -1067,6 +1071,26 @@ static void virtnet_set_affinity(struct virtnet_info *vi)
vi->affinity_hint_set = true;
}
+static int virtnet_cpu_callback(struct notifier_block *nfb,
+ unsigned long action, void *hcpu)
+{
+ struct virtnet_info *vi = container_of(nfb, struct virtnet_info, nb);
+
+ switch(action & ~CPU_TASKS_FROZEN) {
+ case CPU_ONLINE:
+ case CPU_DOWN_FAILED:
+ case CPU_DEAD:
+ virtnet_set_affinity(vi);
+ break;
+ case CPU_DOWN_PREPARE:
+ virtnet_clean_affinity(vi, (long)hcpu);
+ break;
+ default:
+ break;
+ }
+ return NOTIFY_OK;
+}
+
static void virtnet_get_ringparam(struct net_device *dev,
struct ethtool_ringparam *ring)
{
@@ -1541,6 +1565,13 @@ static int virtnet_probe(struct virtio_device *vdev)
}
}
+ vi->nb.notifier_call = &virtnet_cpu_callback;
+ err = register_hotcpu_notifier(&vi->nb);
+ if (err) {
+ pr_debug("virtio_net: registering cpu notifier failed\n");
+ goto free_recv_bufs;
+ }
+
/* Assume link up if device can't report link status,
otherwise get link status from config. */
if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
@@ -1587,6 +1618,8 @@ static void virtnet_remove(struct virtio_device *vdev)
{
struct virtnet_info *vi = vdev->priv;
+ unregister_hotcpu_notifier(&vi->nb);
+
/* Prevent config work handler from accessing the device. */
mutex_lock(&vi->config_lock);
vi->config_enable = false;
--
1.8.1
^ permalink raw reply related
* [PATCH V8 1/3] virtio-net: fix the set affinity bug when CPU IDs are not consecutive
From: Wanlong Gao @ 2013-01-25 9:51 UTC (permalink / raw)
To: linux-kernel
Cc: Rusty Russell, Michael S. Tsirkin, Jason Wang, Eric Dumazet,
David S. Miller, virtualization, netdev, Wanlong Gao
As Michael mentioned, set affinity and select queue will not work very
well when CPU IDs are not consecutive, this can happen with hot unplug.
Fix this bug by traversal the online CPUs, and create a per cpu variable
to find the mapping from CPU to the preferable virtual-queue.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Eric Dumazet <erdnetdev@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
V7->V8:
set vq index unconditionally, make bisect happy,
make sure don't introduce bug. (Jason)
V6->V7:
serialize virtnet_set_queues to avoid a race with cpu hotplug (Jason)
V5->V6:
remove {get|put}_online_cpus from virtnet_del_vqs (Jason)
V4->V5:
Add get/put_online_cpus to avoid CPUs go up and down during operations (Rusty)
V3->V4:
move vq_index into virtnet_info (Jason)
change the mapping value when not setting affinity (Jason)
address the comments about select_queue (Rusty)
drivers/net/virtio_net.c | 67 ++++++++++++++++++++++++++++++++++++++----------
1 file changed, 54 insertions(+), 13 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a6fcf15..fda214a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -123,6 +123,9 @@ struct virtnet_info {
/* Does the affinity hint is set for virtqueues? */
bool affinity_hint_set;
+
+ /* Per-cpu variable to show the mapping from CPU to virtqueue */
+ int __percpu *vq_index;
};
struct skb_vnet_hdr {
@@ -1016,6 +1019,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
{
int i;
+ int cpu;
/* In multiqueue mode, when the number of cpu is equal to the number of
* queue pairs, we let the queue pairs to be private to one cpu by
@@ -1023,22 +1027,40 @@ static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
*/
if ((vi->curr_queue_pairs == 1 ||
vi->max_queue_pairs != num_online_cpus()) && set) {
- if (vi->affinity_hint_set)
+ if (vi->affinity_hint_set) {
set = false;
- else
+ } else {
+ i = 0;
+ for_each_online_cpu(cpu)
+ *per_cpu_ptr(vi->vq_index, cpu) =
+ ++i % vi->curr_queue_pairs;
return;
+ }
}
- for (i = 0; i < vi->max_queue_pairs; i++) {
- int cpu = set ? i : -1;
- virtqueue_set_affinity(vi->rq[i].vq, cpu);
- virtqueue_set_affinity(vi->sq[i].vq, cpu);
- }
+ if (set) {
+ i = 0;
+ for_each_online_cpu(cpu) {
+ virtqueue_set_affinity(vi->rq[i].vq, cpu);
+ virtqueue_set_affinity(vi->sq[i].vq, cpu);
+ *per_cpu_ptr(vi->vq_index, cpu) = i;
+ i++;
+ }
- if (set)
vi->affinity_hint_set = true;
- else
+ } else {
+ for(i = 0; i < vi->max_queue_pairs; i++) {
+ virtqueue_set_affinity(vi->rq[i].vq, -1);
+ virtqueue_set_affinity(vi->sq[i].vq, -1);
+ }
+
+ i = 0;
+ for_each_online_cpu(cpu)
+ *per_cpu_ptr(vi->vq_index, cpu) =
+ ++i % vi->curr_queue_pairs;
+
vi->affinity_hint_set = false;
+ }
}
static void virtnet_get_ringparam(struct net_device *dev,
@@ -1082,6 +1104,7 @@ static int virtnet_set_channels(struct net_device *dev,
if (queue_pairs > vi->max_queue_pairs)
return -EINVAL;
+ get_online_cpus();
err = virtnet_set_queues(vi, queue_pairs);
if (!err) {
netif_set_real_num_tx_queues(dev, queue_pairs);
@@ -1089,6 +1112,7 @@ static int virtnet_set_channels(struct net_device *dev,
virtnet_set_affinity(vi, true);
}
+ put_online_cpus();
return err;
}
@@ -1127,12 +1151,19 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
/* To avoid contending a lock hold by a vcpu who would exit to host, select the
* txq based on the processor id.
- * TODO: handle cpu hotplug.
*/
static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
{
- int txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) :
- smp_processor_id();
+ int txq;
+ struct virtnet_info *vi = netdev_priv(dev);
+
+ if (skb_rx_queue_recorded(skb)) {
+ txq = skb_get_rx_queue(skb);
+ } else {
+ txq = *__this_cpu_ptr(vi->vq_index);
+ if (txq == -1)
+ txq = 0;
+ }
while (unlikely(txq >= dev->real_num_tx_queues))
txq -= dev->real_num_tx_queues;
@@ -1371,7 +1402,10 @@ static int init_vqs(struct virtnet_info *vi)
if (ret)
goto err_free;
+ get_online_cpus();
virtnet_set_affinity(vi, true);
+ put_online_cpus();
+
return 0;
err_free:
@@ -1453,6 +1487,10 @@ static int virtnet_probe(struct virtio_device *vdev)
if (vi->stats == NULL)
goto free;
+ vi->vq_index = alloc_percpu(int);
+ if (vi->vq_index == NULL)
+ goto free_stats;
+
mutex_init(&vi->config_lock);
vi->config_enable = true;
INIT_WORK(&vi->config_work, virtnet_config_changed_work);
@@ -1476,7 +1514,7 @@ static int virtnet_probe(struct virtio_device *vdev)
/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
err = init_vqs(vi);
if (err)
- goto free_stats;
+ goto free_index;
netif_set_real_num_tx_queues(dev, 1);
netif_set_real_num_rx_queues(dev, 1);
@@ -1520,6 +1558,8 @@ free_recv_bufs:
free_vqs:
cancel_delayed_work_sync(&vi->refill);
virtnet_del_vqs(vi);
+free_index:
+ free_percpu(vi->vq_index);
free_stats:
free_percpu(vi->stats);
free:
@@ -1554,6 +1594,7 @@ static void virtnet_remove(struct virtio_device *vdev)
flush_work(&vi->config_work);
+ free_percpu(vi->vq_index);
free_percpu(vi->stats);
free_netdev(vi->dev);
}
--
1.8.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox