* Re: Network performance with small packets - continued
From: Tom Lendacky @ 2011-03-09 16:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Rusty Russell, Krishna Kumar2, David Miller, kvm,
netdev, steved
In-Reply-To: <20110309071558.GA25757@redhat.com>
On Wednesday, March 09, 2011 01:15:58 am Michael S. Tsirkin wrote:
> On Mon, Mar 07, 2011 at 04:31:41PM -0600, Tom Lendacky wrote:
> > We've been doing some more experimenting with the small packet network
> > performance problem in KVM. I have a different setup than what Steve D.
> > was using so I re-baselined things on the kvm.git kernel on both the
> > host and guest with a 10GbE adapter. I also made use of the
> > virtio-stats patch.
> >
> > The virtual machine has 2 vCPUs, 8GB of memory and two virtio network
> > adapters (the first connected to a 1GbE adapter and a LAN, the second
> > connected to a 10GbE adapter that is direct connected to another system
> > with the same 10GbE adapter) running the kvm.git kernel. The test was a
> > TCP_RR test with 100 connections from a baremetal client to the KVM
> > guest using a 256 byte message size in both directions.
> >
> > I used the uperf tool to do this after verifying the results against
> > netperf. Uperf allows the specification of the number of connections as
> > a parameter in an XML file as opposed to launching, in this case, 100
> > separate instances of netperf.
> >
> > Here is the baseline for baremetal using 2 physical CPUs:
> > Txn Rate: 206,389.59 Txn/Sec, Pkt Rate: 410,048 Pkts/Sec
> > TxCPU: 7.88% RxCPU: 99.41%
> >
> > To be sure to get consistent results with KVM I disabled the
> > hyperthreads, pinned the qemu-kvm process, vCPUs, vhost thread and
> > ethernet adapter interrupts (this resulted in runs that differed by only
> > about 2% from lowest to highest). The fact that pinning is required to
> > get consistent results is a different problem that we'll have to look
> > into later...
> >
> > Here is the KVM baseline (average of six runs):
> > Txn Rate: 87,070.34 Txn/Sec, Pkt Rate: 172,992 Pkts/Sec
> > Exits: 148,444.58 Exits/Sec
> > TxCPU: 2.40% RxCPU: 99.35%
> >
> > About 42% of baremetal.
>
> Can you add interrupt stats as well please?
Yes I can. Just the guest interrupts for the virtio device?
>
> > empty. So I coded a quick patch to delay freeing of the used Tx buffers
> > until more than half the ring was used (I did not test this under a
> > stream condition so I don't know if this would have a negative impact).
> > Here are the results
> >
> > from delaying the freeing of used Tx buffers (average of six runs):
> > Txn Rate: 90,886.19 Txn/Sec, Pkt Rate: 180,571 Pkts/Sec
> > Exits: 142,681.67 Exits/Sec
> > TxCPU: 2.78% RxCPU: 99.36%
> >
> > About a 4% increase over baseline and about 44% of baremetal.
>
> Hmm, I am not sure what you mean by delaying freeing.
In the start_xmit function of virtio_net.c the first thing done is to free any
used entries from the ring. I patched the code to track the number of used tx
ring entries and only free the used entries when they are greater than half
the capacity of the ring (similar to the way the rx ring is re-filled).
> I think we do have a problem that free_old_xmit_skbs
> tries to flush out the ring aggressively:
> it always polls until the ring is empty,
> so there could be bursts of activity where
> we spend a lot of time flushing the old entries
> before e.g. sending an ack, resulting in
> latency bursts.
>
> Generally we'll need some smarter logic,
> but with indirect at the moment we can just poll
> a single packet after we post a new one, and be done with it.
> Is your patch something like the patch below?
> Could you try mine as well please?
Yes, I'll try the patch and post the results.
>
> > This spread out the kick_notify but still resulted in alot of them. I
> > decided to build on the delayed Tx buffer freeing and code up an
> > "ethtool" like coalescing patch in order to delay the kick_notify until
> > there were at least 5 packets on the ring or 2000 usecs, whichever
> > occurred first. Here are the
> >
> > results of delaying the kick_notify (average of six runs):
> > Txn Rate: 107,106.36 Txn/Sec, Pkt Rate: 212,796 Pkts/Sec
> > Exits: 102,587.28 Exits/Sec
> > TxCPU: 3.03% RxCPU: 99.33%
> >
> > About a 23% increase over baseline and about 52% of baremetal.
> >
> > Running the perf command against the guest I noticed almost 19% of the
> > time being spent in _raw_spin_lock. Enabling lockstat in the guest
> > showed alot of contention in the "irq_desc_lock_class". Pinning the
> > virtio1-input interrupt to a single cpu in the guest and re-running the
> > last test resulted in
> >
> > tremendous gains (average of six runs):
> > Txn Rate: 153,696.59 Txn/Sec, Pkt Rate: 305,358 Pkgs/Sec
> > Exits: 62,603.37 Exits/Sec
> > TxCPU: 3.73% RxCPU: 98.52%
> >
> > About a 77% increase over baseline and about 74% of baremetal.
> >
> > Vhost is receiving a lot of notifications for packets that are to be
> > transmitted (over 60% of the packets generate a kick_notify). Also, it
> > looks like vhost is sending a lot of notifications for packets it has
> > received before the guest can get scheduled to disable notifications and
> > begin processing the packets
>
> Hmm, is this really what happens to you? The effect would be that guest
> gets an interrupt while notifications are disabled in guest, right? Could
> you add a counter and check this please?
The disabling of the interrupt/notifications is done by the guest. So the
guest has to get scheduled and handle the notification before it disables
them. The vhost_signal routine will keep injecting an interrupt until this
happens causing the contention in the guest. I'll try the patches you specify
below and post the results. They look like they should take care of this
issue.
>
> Another possible thing to try would be these old patches to publish used
> index from guest to make sure this double interrupt does not happen:
> [PATCHv2] virtio: put last seen used index into ring itself
> [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
>
> > resulting in some lock contention in the guest (and
> > high interrupt rates).
> >
> > Some thoughts for the transmit path... can vhost be enhanced to do some
> > adaptive polling so that the number of kick_notify events are reduced and
> > replaced by kick_no_notify events?
>
> Worth a try.
>
> > Comparing the transmit path to the receive path, the guest disables
> > notifications after the first kick and vhost re-enables notifications
> > after completing processing of the tx ring.
>
> Is this really what happens? I though the host disables notifications
> after the first kick.
Yup, sorry for the confusion. The kick is done by the guest and then vhost
disables notifications. Maybe a similar approach to the above patches of
checking the used index in the virtio_net driver could also help here?
>
> > Can a similar thing be done for the
> >
> > receive path? Once vhost sends the first notification for a received
> > packet it can disable notifications and let the guest re-enable
> > notifications when it has finished processing the receive ring. Also,
> > can the virtio-net driver do some adaptive polling (or does napi take
> > care of that for the guest)?
>
> Worth a try. I don't think napi does anything like this.
>
> > Running the same workload on the same configuration with a different
> > hypervisor results in performance that is almost equivalent to baremetal
> > without doing any pinning.
> >
> > Thanks,
> > Tom Lendacky
>
> There's no need to flush out all used buffers
> before we post more for transmit: with indirect,
> just a single one is enough. Without indirect we'll
> need more possibly, but just for testing this should
> be enough.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> Note: untested.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 82dba5a..ebe3337 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> virtnet_info *vi) struct sk_buff *skb;
> unsigned int len, tot_sgs = 0;
>
> - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> + tot_sgs = 2+MAX_SKB_FRAGS;
> dev_kfree_skb_any(skb);
> }
> return tot_sgs;
> @@ -576,9 +576,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev) struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> - /* Free up any pending old buffers before queueing new ones. */
> - free_old_xmit_skbs(vi);
> -
> /* Try to transmit */
> capacity = xmit_skb(vi, skb);
>
> @@ -605,6 +602,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev) skb_orphan(skb);
> nf_reset(skb);
>
> + /* Free up any old buffers so we can queue new ones. */
> + if (capacity < 2+MAX_SKB_FRAGS)
> + capacity += free_old_xmit_skbs(vi);
> +
> /* Apparently nice girls don't return TX_BUSY; stop the queue
> * before it gets out of hand. Naturally, this wastes entries. */
> if (capacity < 2+MAX_SKB_FRAGS) {
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" 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: Network performance with small packets - continued
From: Shirley Ma @ 2011-03-09 15:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Krishna Kumar2, David Miller, kvm, netdev, steved,
Tom Lendacky
In-Reply-To: <20110309071558.GA25757@redhat.com>
On Wed, 2011-03-09 at 09:15 +0200, Michael S. Tsirkin wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 82dba5a..ebe3337 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> virtnet_info *vi)
> struct sk_buff *skb;
> unsigned int len, tot_sgs = 0;
>
> - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> + tot_sgs = 2+MAX_SKB_FRAGS;
> dev_kfree_skb_any(skb);
> }
> return tot_sgs;
Return value should be different based on indirect or direct buffers
here?
> @@ -576,9 +576,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> - /* Free up any pending old buffers before queueing new ones.
> */
> - free_old_xmit_skbs(vi);
> -
> /* Try to transmit */
> capacity = xmit_skb(vi, skb);
>
> @@ -605,6 +602,10 @@ static netdev_tx_t start_xmit(struct sk_buff
> *skb, struct net_device *dev)
> skb_orphan(skb);
> nf_reset(skb);
>
> + /* Free up any old buffers so we can queue new ones. */
> + if (capacity < 2+MAX_SKB_FRAGS)
> + capacity += free_old_xmit_skbs(vi);
> +
> /* Apparently nice girls don't return TX_BUSY; stop the queue
> * before it gets out of hand. Naturally, this wastes
> entries. */
> if (capacity < 2+MAX_SKB_FRAGS) {
I tried a similar patch before, it didn't help much on TCP stream
performance. But I didn't try multiple stream TCP_RR.
Shirley
^ permalink raw reply
* Re: Network performance with small packets
From: Shirley Ma @ 2011-03-09 15:42 UTC (permalink / raw)
To: habanero
Cc: Rusty Russell, Michael S. Tsirkin, Krishna Kumar2, David Miller,
kvm, netdev, steved, Tom Lendacky
In-Reply-To: <1299637278.13202.61.camel@localhost.localdomain>
On Tue, 2011-03-08 at 20:21 -0600, Andrew Theurer wrote:
> Tom L has started using Rusty's patches and found some interesting
> results, sent yesterday:
> http://marc.info/?l=kvm&m=129953710930124&w=2
Thanks. Very good experimental. I have been struggling with guest/vhost
optimization work for a while. I created different experimental patches,
performance results really depends on workloads.
Based on the discussions and findings, seems that to improve
virtio_net/vhost optimization work, we really need to collect more
statistics data on both virtio_net and vhost for both TX and RX.
A way to filter number of guest exits, I/O exits, irq injections in
guest networking stacks only would be helpful.
Thanks
Shirley
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Nicolas de Pesloüan @ 2011-03-09 15:28 UTC (permalink / raw)
To: Jiri Pirko
Cc: Andy Gospodarek, netdev, davem, shemminger, kaber, fubar,
eric.dumazet
In-Reply-To: <20110309150939.GA9013@psychotron.brq.redhat.com>
Le 09/03/2011 16:09, Jiri Pirko a écrit :
> Wed, Mar 09, 2011 at 03:49:49PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 09/03/2011 08:45, Jiri Pirko a écrit :
>>> Tue, Mar 08, 2011 at 10:44:37PM CET, nicolas.2p.debian@gmail.com wrote:
>>>> Le 08/03/2011 14:42, Andy Gospodarek a écrit :
>>>>> I'm pretty sure this patch will have the same catastrophic problem your
>>>>> last one did. By cloning and setting skb2->dev = orig_dev you just
>>>>> inserted a frame identical to the one we received right back into the
>>>>> stack. It only took a few minutes for my box to melt as one frame on
>>>>> the wire will cause an infinite number of frames to be received by the
>>>>> stack.
>>>>
>>>> I agree with Andy. We still keep one reinject (netif_rx), which is
>>>> probably better that two (__netif_receive_skb), but not enough.
>>>>
>>>> I really think we need a general framework for late delivery of final
>>>> packets to packet handler registered somewhere in the rx_handler
>>>> path.
>>>>
>>>> Jiri, is this patch the one you announced as "I have some kind nice
>>>> solution in mind and I'm going to submit that as a patch later (too
>>>> many patches are in the wind atm)" ?
>>>
>>>
>>> I did not had time to verify my thought yet but I think that the only
>>> think needed against my original patch (bonding: move processing of recv
>>> handlers into handle_frame()) is ro remove vlan_on_bond_hook, period.
>>>
>>> Because all incoming arps are seen by bond_handle_frame =>
>>> bond->recv_probe , even vlan ones - that would make eth0-bond0-bond0.5
>>> work and eth0-bond0-br0-bond0.5 as well. But again, need to verify this.
>>
>> All incoming ARPs are seen by bond_handle_frame, even vlan ones, definitely true.
>>
>> But as some of them *are* vlan ones, you will have to untag those "by hand" inside bond_handle_frame.
>
> Hmm. For hw vlan accel this would not be needed.
Agreed.
> But for non-hw-vlan-accel the frame is wrapped when going thru handle_frame. And yes, in that
> case untagging would be necessary. Unless the vlan untagging happening now in ptype_base handler
> is moved in rx path somewhere before __netif_receive_skb.
Can't it me moved not before, but inside __netif_receive_skb, as a rx_handler?
At the time we setup eth0.100, we can arrange for a vlan_untag rx_handler to be registered on eth0.
This is exactly the way it works now for macvlan. Should it be possible (and desirable) for vlan too?
This might re-open the discussion about the need for several rx_handlers per interface, but that is
another story.
Also, moving it before __netif_receive_skb would cause every protocol handlers to receive the frame
untagged. At least protocol sniffers registered at ptype_all may want to receive the tagged frame,
for diagnostic purpose.
That would result in two things:
>
> 1) Bond would be able to scope vlan packets
> 2) The rx path for hw-vlan-accel and non-hw-vlan-accel would be the same
> (should be desirable + one reinjection would be avoided for
> non-hw-vlan-accel)
Agreed, but moving it inside __netif_receive_skb should have the same effects. If we move
non-hw-accel-vlan to a rx_handler, the skb would be COW before being untagged only if shared. This
sounds good to me.
Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Jiri Pirko @ 2011-03-09 15:09 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Andy Gospodarek, netdev, davem, shemminger, kaber, fubar,
eric.dumazet
In-Reply-To: <4D77938D.3080408@gmail.com>
Wed, Mar 09, 2011 at 03:49:49PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 09/03/2011 08:45, Jiri Pirko a écrit :
>>Tue, Mar 08, 2011 at 10:44:37PM CET, nicolas.2p.debian@gmail.com wrote:
>>>Le 08/03/2011 14:42, Andy Gospodarek a écrit :
>>>>I'm pretty sure this patch will have the same catastrophic problem your
>>>>last one did. By cloning and setting skb2->dev = orig_dev you just
>>>>inserted a frame identical to the one we received right back into the
>>>>stack. It only took a few minutes for my box to melt as one frame on
>>>>the wire will cause an infinite number of frames to be received by the
>>>>stack.
>>>
>>>I agree with Andy. We still keep one reinject (netif_rx), which is
>>>probably better that two (__netif_receive_skb), but not enough.
>>>
>>>I really think we need a general framework for late delivery of final
>>>packets to packet handler registered somewhere in the rx_handler
>>>path.
>>>
>>>Jiri, is this patch the one you announced as "I have some kind nice
>>>solution in mind and I'm going to submit that as a patch later (too
>>>many patches are in the wind atm)" ?
>>
>>
>>I did not had time to verify my thought yet but I think that the only
>>think needed against my original patch (bonding: move processing of recv
>>handlers into handle_frame()) is ro remove vlan_on_bond_hook, period.
>>
>>Because all incoming arps are seen by bond_handle_frame =>
>>bond->recv_probe , even vlan ones - that would make eth0-bond0-bond0.5
>>work and eth0-bond0-br0-bond0.5 as well. But again, need to verify this.
>
>All incoming ARPs are seen by bond_handle_frame, even vlan ones, definitely true.
>
>But as some of them *are* vlan ones, you will have to untag those "by hand" inside bond_handle_frame.
Hmm. For hw vlan accel this would not be needed. But for
non-hw-vlan-accel the frame is wrapped when going thru handle_frame.
And yes, in that case untagging would be necessary. Unless the vlan
untagging happening now in ptype_base handler is moved in rx path
somewhere before __netif_receive_skb. That would result in two things:
1) Bond would be able to scope vlan packets
2) The rx path for hw-vlan-accel and non-hw-vlan-accel would be the same
(should be desirable + one reinjection would be avoided for
non-hw-vlan-accel)
>
>It might work, but I wouldn't support the idea, for two reasons :
>
>- It would duplicate code, or at least, duplicate places where untagging happens.
>- It would cause some vlan hacks to sit inside bond, which is not
>nice from a layering point of view. You recently advocated against
>breaking layering and you are right. We should avoid putting X
>related stuff into Y part of the network stack, as much as possible.
>
>Again, I think we should try and find a generic way to have the final
>frame delivered to whoever needs it. And this way should not be
>limited to "vlan untagging" nor to bonding.
>
> Nicolas.
^ permalink raw reply
* Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state
From: Américo Wang @ 2011-03-09 15:09 UTC (permalink / raw)
To: Adam Majer
Cc: netdev, Stephen Hemminger, David S. Miller, Alexey Kuznetsov,
Pekka Savola (ipv6), James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, bridge, linux-kernel
In-Reply-To: <20110306051833.GA3098@mira.lan.galacticasoftware.com>
On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
>
>IPv6 address autoconfiguration relies on an UP interface to processes
>traffic normally. A bridge that is UP enters LEARNING state and this
>state "eats" any Router Advertising packets sent to the
>bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
>when it changes state allows autoconfiguration code to retry
>querying router information.
>
...
>+static void br_port_change_notifier_handler(struct work_struct *work)
>+{
>+ struct net_bridge *br = container_of(work,
>+ struct net_bridge,
>+ change_notification_worker);
>+
>+ rtnl_lock();
>+ netdev_state_change(br->dev);
>+ rtnl_unlock();
>+}
>+
Do you really want user-space to get this notification too?
Why do you put it into a workqueue? Maybe it has to be called in
process-context?
Thanks.
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Nicolas de Pesloüan @ 2011-03-09 14:49 UTC (permalink / raw)
To: Jiri Pirko
Cc: Andy Gospodarek, netdev, davem, shemminger, kaber, fubar,
eric.dumazet
In-Reply-To: <20110309074547.GA2808@psychotron.redhat.com>
Le 09/03/2011 08:45, Jiri Pirko a écrit :
> Tue, Mar 08, 2011 at 10:44:37PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 08/03/2011 14:42, Andy Gospodarek a écrit :
>>> I'm pretty sure this patch will have the same catastrophic problem your
>>> last one did. By cloning and setting skb2->dev = orig_dev you just
>>> inserted a frame identical to the one we received right back into the
>>> stack. It only took a few minutes for my box to melt as one frame on
>>> the wire will cause an infinite number of frames to be received by the
>>> stack.
>>
>> I agree with Andy. We still keep one reinject (netif_rx), which is
>> probably better that two (__netif_receive_skb), but not enough.
>>
>> I really think we need a general framework for late delivery of final
>> packets to packet handler registered somewhere in the rx_handler
>> path.
>>
>> Jiri, is this patch the one you announced as "I have some kind nice
>> solution in mind and I'm going to submit that as a patch later (too
>> many patches are in the wind atm)" ?
>
>
> I did not had time to verify my thought yet but I think that the only
> think needed against my original patch (bonding: move processing of recv
> handlers into handle_frame()) is ro remove vlan_on_bond_hook, period.
>
> Because all incoming arps are seen by bond_handle_frame =>
> bond->recv_probe , even vlan ones - that would make eth0-bond0-bond0.5
> work and eth0-bond0-br0-bond0.5 as well. But again, need to verify this.
All incoming ARPs are seen by bond_handle_frame, even vlan ones, definitely true.
But as some of them *are* vlan ones, you will have to untag those "by hand" inside bond_handle_frame.
It might work, but I wouldn't support the idea, for two reasons :
- It would duplicate code, or at least, duplicate places where untagging happens.
- It would cause some vlan hacks to sit inside bond, which is not nice from a layering point of
view. You recently advocated against breaking layering and you are right. We should avoid putting X
related stuff into Y part of the network stack, as much as possible.
Again, I think we should try and find a generic way to have the final frame delivered to whoever
needs it. And this way should not be limited to "vlan untagging" nor to bonding.
Nicolas.
^ permalink raw reply
* [PATCH] Davinci: Do not reset EMAC TX overruns counter on read
From: Thomas Lange @ 2011-03-09 14:41 UTC (permalink / raw)
To: netdev
Cc: davinci-linux-open-source@linux.davincidsp.com, davem, anantgole,
chaithrika
Don't reset tx_fifo_errors when reading out current EMAC stats.
(tx_fifo_errors shows up as TX overruns in netdev stats.)
Without this correction, the old counter value is lost every time
stats are read out.
Signed-off-by: Thomas Lange <thomas@corelatus.se>
---
drivers/net/davinci_emac.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index 2a628d1..922c8c5 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -1730,7 +1730,7 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
emac_read(EMAC_TXCARRIERSENSE);
emac_write(EMAC_TXCARRIERSENSE, stats_clear_mask);
- ndev->stats.tx_fifo_errors = emac_read(EMAC_TXUNDERRUN);
+ ndev->stats.tx_fifo_errors += emac_read(EMAC_TXUNDERRUN);
emac_write(EMAC_TXUNDERRUN, stats_clear_mask);
return &ndev->stats;
--
1.7.2.5
^ permalink raw reply related
* Re: Bug inkvm_set_irq
From: Michael S. Tsirkin @ 2011-03-09 13:59 UTC (permalink / raw)
To: Jean-Philippe Menil; +Cc: netdev, kvm, virtualization
In-Reply-To: <4D777CDA.3050500@univ-nantes.fr>
On Wed, Mar 09, 2011 at 02:12:58PM +0100, Jean-Philippe Menil wrote:
> Le 09/03/2011 14:00, Michael S. Tsirkin a écrit :
> >On Wed, Mar 09, 2011 at 01:28:43PM +0100, Jean-Philippe Menil wrote:
> >>Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
> >>>On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
> >>>>Yes, it's a 2.6.37.2 kernel.
> >>>OK, here's a debugging patch.
> >>>Please run with slab debugging as previously until you see
> >>>'eventfd bug detected!' in dmesg or until there is a crash.
> >>>It might be also useful to enable timestampts on printk with
> >>> Symbol: PRINTK_TIME [=y]
> >>> │ Type : boolean
> >>> │ Prompt: Show timing information on printks
> >>>
> >>>once you see the error, please upload the
> >>>full dmesg output somewhere to we can track what
> >>>goes on.
> >>>
> >>>Hopefully there won't be an oops this time which
> >>>should make it easier for you to test (no need to
> >>>reboot).
> >>>
> >>>
> >>>diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> >>>index c1f1e3c..3cb679b 100644
> >>>--- a/virt/kvm/eventfd.c
> >>>+++ b/virt/kvm/eventfd.c
> >>>@@ -32,6 +32,7 @@
> >>> #include<linux/eventfd.h>
> >>> #include<linux/kernel.h>
> >>> #include<linux/slab.h>
> >>>+#include<linux/nmi.h>
> >>>
> >>> #include "iodev.h"
> >>>
> >>>@@ -43,6 +44,8 @@
> >>> * --------------------------------------------------------------------
> >>> */
> >>>
> >>>+#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
> >>>+
> >>> struct _irqfd {
> >>> struct kvm *kvm;
> >>> struct eventfd_ctx *eventfd;
> >>>@@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
> >>> {
> >>> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> >>> struct kvm *kvm = irqfd->kvm;
> >>>+ if (kvm == KVM_BAD_PTR) {
> >>>+ printk(KERN_ERR "Eventfd bug detected!\n");
> >>>+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
> >>>+ work, irqfd, kvm, irqfd->gsi);
> >>>+ trigger_all_cpu_backtrace();
> >>>+ return;
> >>>+ }
> >>>
> >>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
> >>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
> >>>@@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
> >>> struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
> >>> u64 cnt;
> >>>
> >>>+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >>>+ work, irqfd, irqfd->kvm, irqfd->gsi);
> >>> /*
> >>> * Synchronize with the wait-queue and unhook ourselves to prevent
> >>> * further events.
> >>>@@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
> >>> * It is now safe to release the object's resources
> >>> */
> >>> eventfd_ctx_put(irqfd->eventfd);
> >>>+ printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
> >>>+ work, irqfd);
> >>> kfree(irqfd);
> >>> }
> >>>
> >>>@@ -111,6 +125,8 @@ static void
> >>> irqfd_deactivate(struct _irqfd *irqfd)
> >>> {
> >>> BUG_ON(!irqfd_is_active(irqfd));
> >>>+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >>>+ irqfd, irqfd->kvm, irqfd->gsi);
> >>>
> >>> list_del_init(&irqfd->list);
> >>>
> >>>@@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
> >>>
> >>> irqfd->kvm = kvm;
> >>> irqfd->gsi = gsi;
> >>>+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >>>+ irqfd, kvm, gsi);
> >>> INIT_LIST_HEAD(&irqfd->list);
> >>> INIT_WORK(&irqfd->inject, irqfd_inject);
> >>> INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
> >>>@@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
> >>> struct _irqfd *irqfd, *tmp;
> >>> struct eventfd_ctx *eventfd;
> >>>
> >>>+ printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
> >>>+ kvm, gsi);
> >>> eventfd = eventfd_ctx_fdget(fd);
> >>> if (IS_ERR(eventfd))
> >>> return PTR_ERR(eventfd);
> >>>@@ -305,6 +325,7 @@ void
> >>> kvm_irqfd_release(struct kvm *kvm)
> >>> {
> >>> struct _irqfd *irqfd, *tmp;
> >>>+ printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
> >>>
> >>> spin_lock_irq(&kvm->irqfds.lock);
> >>>
> >>>--
> >>>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
> >>Hi,
> >>
> >>I boot the host with the patched kernel yesterday.
> >>No crach until now, but two "Eventfd bug detected!" in the log at
> >>"Mar 9 02:04:31" and "Mar 9 02:15:17"
> >>You can find part of the log at the following adress:
> >>http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
> >>
> >>It a split file of the kern.log (1,4G), so if you need another part
> >>of the log, let me know.
> >>
> >>Thanks for all.
> >>
> >>Regards.
> >Downloading, it's big :)
> >What about some 1000 lines before and after Eventfd bug detected! line?
> >--
> >To unsubscribe from this list: send the line "unsubscribe kvm" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
> Yes, sorry about that.
> I could have split my log into a smaller file.
>
> I was a little afraid of not transmit enough informations, and i was
> a bit wide.
> I hope you can find usefull trace anyway.
>
> Regards.
OK, use after free.
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.593681]
kvm_irqfd_assign(irqfd=ffff88045e8d6230,kvm=ffff88085151c000, gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605359]
kvm_irqfd_deassign(kvm=ffff88085151c000, gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605388]
irqfd_deactivate(irqfd=ffff88045e8d6230,kvm=ffff88085151c000, gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605461]
irqfd_shutdown(work=ffff88045e8d62b0,irqfd=ffff88045e8d6230,kvm=ffff88085151c000,
gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605509]
kfree at irqfd_shutdown(work=ffff88045e8d62b0,irqfd=ffff88045e8d6230)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605588]
Eventfd bug detected!
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605612]
irqfd_inject(work=ffff88045e8d6290,irqfd=ffff88045e8d6230,kvm=6b6b6b6b6b6b6b6b,gsi=1802201963)
and 1802201963 is also 6b6b6b6b.
I suspect that somehow, we get an event from eventfd even
though we did eventfd_ctx_remove_wait_queue.
Could you please try the following patch on top?
When you see Eventfd bug or Wakeup bug, paste some
last lines.
For the full log file - better compress with xz.
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 3cb679b..63aeba5 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -92,6 +92,8 @@ irqfd_shutdown(struct work_struct *work)
* further events.
*/
eventfd_ctx_remove_wait_queue(irqfd->eventfd, &irqfd->wait, &cnt);
+ printk(KERN_ERR "remove wq at %s(work=%p,irqfd=%p)\n", __func__,
+ work, irqfd);
/*
* We know no new events will be scheduled at this point, so block
@@ -142,6 +144,13 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
unsigned long flags = (unsigned long)key;
+ if (irqfd->kvm == KVM_BAD_PTR) {
+ printk(KERN_ERR "Wakeup bug detected! flags 0x%lx\n", flags);
+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
+ &irqfd->inject, irqfd, irqfd->kvm, irqfd->gsi);
+ trigger_all_cpu_backtrace();
+ return 0;
+ }
if (flags & POLLIN)
/* An event has been signaled, inject an interrupt */
schedule_work(&irqfd->inject);
@@ -153,6 +162,9 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
spin_lock_irqsave(&kvm->irqfds.lock, flags);
+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d,active=%d)\n", __func__,
+ &irqfd->inject, irqfd, irqfd->kvm, irqfd->gsi,
+ irqfd_is_active(irqfd));
/*
* We must check if someone deactivated the irqfd before
* we could acquire the irqfds.lock since the item is
--
MST
^ permalink raw reply related
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Neil Horman @ 2011-03-09 13:33 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Andy Gospodarek, Jiri Pirko, netdev, davem, shemminger, kaber,
fubar, eric.dumazet
In-Reply-To: <4D76A345.9040200@gmail.com>
On Tue, Mar 08, 2011 at 10:44:37PM +0100, Nicolas de Pesloüan wrote:
> Le 08/03/2011 14:42, Andy Gospodarek a écrit :
> >I'm pretty sure this patch will have the same catastrophic problem your
> >last one did. By cloning and setting skb2->dev = orig_dev you just
> >inserted a frame identical to the one we received right back into the
> >stack. It only took a few minutes for my box to melt as one frame on
> >the wire will cause an infinite number of frames to be received by the
> >stack.
>
> I agree with Andy. We still keep one reinject (netif_rx), which is
> probably better that two (__netif_receive_skb), but not enough.
>
> I really think we need a general framework for late delivery of
> final packets to packet handler registered somewhere in the
> rx_handler path.
>
> Jiri, is this patch the one you announced as "I have some kind nice
> solution in mind and I'm going to submit that as a patch later (too
> many patches are in the wind atm)" ?
I've not had time to flesh any of this out, but have you considered the use of
privately allocated net namespaces to define the receive order of frames when
doing multiple re-injections through netif_rx/netif_receive_skb. If you
modified the ptype list traversals so validate that dev_net(ptype->dev) ==
dev_net(skb->dev) before delivery, you could allocate private net namespaces in
your virutal master drivers (bridges/bonds/vlans) and place the slave devices in
those spaces, so that only the parent device could receive frames on them.
Of course thats going to have its own set of problems to deal with, but it might
be worth considering.
Neil
> --
> 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: Bug inkvm_set_irq
From: Jean-Philippe Menil @ 2011-03-09 13:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20110309130000.GA30297@redhat.com>
Le 09/03/2011 14:00, Michael S. Tsirkin a écrit :
> On Wed, Mar 09, 2011 at 01:28:43PM +0100, Jean-Philippe Menil wrote:
>> Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
>>> On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
>>>> Yes, it's a 2.6.37.2 kernel.
>>> OK, here's a debugging patch.
>>> Please run with slab debugging as previously until you see
>>> 'eventfd bug detected!' in dmesg or until there is a crash.
>>> It might be also useful to enable timestampts on printk with
>>> Symbol: PRINTK_TIME [=y]
>>> │ Type : boolean
>>> │ Prompt: Show timing information on printks
>>>
>>> once you see the error, please upload the
>>> full dmesg output somewhere to we can track what
>>> goes on.
>>>
>>> Hopefully there won't be an oops this time which
>>> should make it easier for you to test (no need to
>>> reboot).
>>>
>>>
>>> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
>>> index c1f1e3c..3cb679b 100644
>>> --- a/virt/kvm/eventfd.c
>>> +++ b/virt/kvm/eventfd.c
>>> @@ -32,6 +32,7 @@
>>> #include<linux/eventfd.h>
>>> #include<linux/kernel.h>
>>> #include<linux/slab.h>
>>> +#include<linux/nmi.h>
>>>
>>> #include "iodev.h"
>>>
>>> @@ -43,6 +44,8 @@
>>> * --------------------------------------------------------------------
>>> */
>>>
>>> +#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
>>> +
>>> struct _irqfd {
>>> struct kvm *kvm;
>>> struct eventfd_ctx *eventfd;
>>> @@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
>>> {
>>> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
>>> struct kvm *kvm = irqfd->kvm;
>>> + if (kvm == KVM_BAD_PTR) {
>>> + printk(KERN_ERR "Eventfd bug detected!\n");
>>> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
>>> + work, irqfd, kvm, irqfd->gsi);
>>> + trigger_all_cpu_backtrace();
>>> + return;
>>> + }
>>>
>>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
>>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
>>> @@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
>>> struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
>>> u64 cnt;
>>>
>>> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
>>> + work, irqfd, irqfd->kvm, irqfd->gsi);
>>> /*
>>> * Synchronize with the wait-queue and unhook ourselves to prevent
>>> * further events.
>>> @@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
>>> * It is now safe to release the object's resources
>>> */
>>> eventfd_ctx_put(irqfd->eventfd);
>>> + printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
>>> + work, irqfd);
>>> kfree(irqfd);
>>> }
>>>
>>> @@ -111,6 +125,8 @@ static void
>>> irqfd_deactivate(struct _irqfd *irqfd)
>>> {
>>> BUG_ON(!irqfd_is_active(irqfd));
>>> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
>>> + irqfd, irqfd->kvm, irqfd->gsi);
>>>
>>> list_del_init(&irqfd->list);
>>>
>>> @@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
>>>
>>> irqfd->kvm = kvm;
>>> irqfd->gsi = gsi;
>>> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
>>> + irqfd, kvm, gsi);
>>> INIT_LIST_HEAD(&irqfd->list);
>>> INIT_WORK(&irqfd->inject, irqfd_inject);
>>> INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
>>> @@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
>>> struct _irqfd *irqfd, *tmp;
>>> struct eventfd_ctx *eventfd;
>>>
>>> + printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
>>> + kvm, gsi);
>>> eventfd = eventfd_ctx_fdget(fd);
>>> if (IS_ERR(eventfd))
>>> return PTR_ERR(eventfd);
>>> @@ -305,6 +325,7 @@ void
>>> kvm_irqfd_release(struct kvm *kvm)
>>> {
>>> struct _irqfd *irqfd, *tmp;
>>> + printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
>>>
>>> spin_lock_irq(&kvm->irqfds.lock);
>>>
>>> --
>>> 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
>> Hi,
>>
>> I boot the host with the patched kernel yesterday.
>> No crach until now, but two "Eventfd bug detected!" in the log at
>> "Mar 9 02:04:31" and "Mar 9 02:15:17"
>> You can find part of the log at the following adress:
>> http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
>>
>> It a split file of the kern.log (1,4G), so if you need another part
>> of the log, let me know.
>>
>> Thanks for all.
>>
>> Regards.
> Downloading, it's big :)
> What about some 1000 lines before and after Eventfd bug detected! line?
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes, sorry about that.
I could have split my log into a smaller file.
I was a little afraid of not transmit enough informations, and i was a
bit wide.
I hope you can find usefull trace anyway.
Regards.
--
Jean-Philippe Menil - Pôle réseau Service IRTS
DSI Université de Nantes
jean-philippe.menil@univ-nantes.fr
Tel : 02.53.48.49.27 - Fax : 02.53.48.49.09
^ permalink raw reply
* Re: [PATCH 1/2] net: Allow no-cache copy from user on transmit
From: Michał Mirosław @ 2011-03-09 13:04 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev
In-Reply-To: <alpine.DEB.2.00.1103082053350.1433@pokey.mtv.corp.google.com>
2011/3/9 Tom Herbert <therbert@google.com>:
> This patch uses __copy_from_user_nocache (from skb_copy_to_page)
> on transmit to bypass data cache for a performance improvement.
> This functionality is configurable per device using ethtool, the
> device must also be doing TX csum offload to enable. It seems
> reasonable to set this when the device does not copy or
> otherwise touch the data.
>
> This patch was tested using 200 instances of netperf TCP_RR with
> 1400 byte request and one byte reply. Platform is 16 core AMD.
>
> No-cache copy disabled:
> 672703 tps, 97.13% client utilization
> 50/90/99% latency 244.31 484.205 1028.41
>
> No-cache copy enabled:
> 702113 tps, 96.16% client utilization,
> 50/90/99% latency 238.56 467.56 956.955
>
> This seems to provide a nice little performance improvement and is
> consistent in the tests I ran. Presumably, this would provide
> the greatest benfits in the presence of an application workload
> stressing the cache and a lot of transmit data happening. I don't
> yet see a downside to using this.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> include/linux/ethtool.h | 4 ++++
> include/linux/netdevice.h | 4 +++-
> include/net/sock.h | 5 +++++
> net/core/ethtool.c | 11 ++++++++++-
> 4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index aac3e2e..2cf3cc9 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -833,6 +833,10 @@ struct ethtool_ops {
> #define ETHTOOL_GFEATURES 0x0000003a /* Get device offload settings */
> #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */
>
> +#define ETHTOOL_GNCCOPY 0x0000003c /* Get no-cache-copy enable
> + * (ethtool_value). */
> +#define ETHTOOL_SNCCOPY 0x0000003d /* Set no-cache-copy enable
> + * (ethtool_value). */
Please don't add new get/set ops for netdev features - G/SFEATURES
handle them all.
Proper way is to modify NETIF_F_ETHTOOL_BITS, netdev_fix_features()
and, if needed for now,
legacy get/set ops (they will go away, eventually).
> /* compatibility with older code */
> #define SPARC_ETH_GSET ETHTOOL_GSET
> #define SPARC_ETH_SSET ETHTOOL_SSET
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 71563e7..1b72f29 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -978,6 +978,7 @@ struct net_device {
> #define NETIF_F_NTUPLE (1 << 27) /* N-tuple filters supported */
> #define NETIF_F_RXHASH (1 << 28) /* Receive hashing offload */
> #define NETIF_F_RXCSUM (1 << 29) /* Receive checksumming offload */
> +#define NETIF_F_NOCACHE_COPY (1 << 30) /* Use no-cache copyfromuser */
>
> /* Segmentation offload features */
> #define NETIF_F_GSO_SHIFT 16
> @@ -1020,7 +1021,8 @@ struct net_device {
> NETIF_F_FRAGLIST)
>
> /* changeable features with no special hardware requirements */
> -#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO)
> +#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO | \
> + NETIF_F_NOCACHE_COPY)
>
> /* Interface index. Unique device identifier */
> int ifindex;
> diff --git a/include/net/sock.h b/include/net/sock.h
> index da0534d..55b1385 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1401,6 +1401,11 @@ static inline int skb_copy_to_page(struct sock *sk, char __user *from,
> if (err)
> return err;
> skb->csum = csum_block_add(skb->csum, csum, skb->len);
> + } else if (sk->sk_route_caps & NETIF_F_NOCACHE_COPY) {
> + if (!access_ok(VERIFY_READ, from, copy)
> + || __copy_from_user_nocache(page_address(page) + off,
> + from, copy))
> + return -EFAULT;
> } else if (copy_from_user(page_address(page) + off, from, copy))
> return -EFAULT;
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..a8b2638 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -328,6 +328,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
> /* NETIF_F_NETNS_LOCAL */ "netns-local",
> /* NETIF_F_GRO */ "rx-gro",
> /* NETIF_F_LRO */ "rx-lro",
> + /* NETIF_F_NOCACHE_COPY */ "tx-nocache-copy",
>
> /* NETIF_F_TSO */ "tx-tcp-segmentation",
> /* NETIF_F_UFO */ "tx-udp-fragmentation",
You can't put the strings in the middle of this array. Positions in
the array are dependent on flag bit position.
[...]
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Bug inkvm_set_irq
From: Michael S. Tsirkin @ 2011-03-09 13:00 UTC (permalink / raw)
To: Jean-Philippe Menil; +Cc: netdev, kvm, virtualization
In-Reply-To: <4D77727B.2000207@univ-nantes.fr>
On Wed, Mar 09, 2011 at 01:28:43PM +0100, Jean-Philippe Menil wrote:
> Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
> >On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
> >>Yes, it's a 2.6.37.2 kernel.
> >OK, here's a debugging patch.
> >Please run with slab debugging as previously until you see
> >'eventfd bug detected!' in dmesg or until there is a crash.
> >It might be also useful to enable timestampts on printk with
> > Symbol: PRINTK_TIME [=y]
> > │ Type : boolean
> > │ Prompt: Show timing information on printks
> >
> >once you see the error, please upload the
> >full dmesg output somewhere to we can track what
> >goes on.
> >
> >Hopefully there won't be an oops this time which
> >should make it easier for you to test (no need to
> >reboot).
> >
> >
> >diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> >index c1f1e3c..3cb679b 100644
> >--- a/virt/kvm/eventfd.c
> >+++ b/virt/kvm/eventfd.c
> >@@ -32,6 +32,7 @@
> > #include<linux/eventfd.h>
> > #include<linux/kernel.h>
> > #include<linux/slab.h>
> >+#include<linux/nmi.h>
> >
> > #include "iodev.h"
> >
> >@@ -43,6 +44,8 @@
> > * --------------------------------------------------------------------
> > */
> >
> >+#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
> >+
> > struct _irqfd {
> > struct kvm *kvm;
> > struct eventfd_ctx *eventfd;
> >@@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
> > {
> > struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> > struct kvm *kvm = irqfd->kvm;
> >+ if (kvm == KVM_BAD_PTR) {
> >+ printk(KERN_ERR "Eventfd bug detected!\n");
> >+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
> >+ work, irqfd, kvm, irqfd->gsi);
> >+ trigger_all_cpu_backtrace();
> >+ return;
> >+ }
> >
> > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
> > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
> >@@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
> > struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
> > u64 cnt;
> >
> >+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >+ work, irqfd, irqfd->kvm, irqfd->gsi);
> > /*
> > * Synchronize with the wait-queue and unhook ourselves to prevent
> > * further events.
> >@@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
> > * It is now safe to release the object's resources
> > */
> > eventfd_ctx_put(irqfd->eventfd);
> >+ printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
> >+ work, irqfd);
> > kfree(irqfd);
> > }
> >
> >@@ -111,6 +125,8 @@ static void
> > irqfd_deactivate(struct _irqfd *irqfd)
> > {
> > BUG_ON(!irqfd_is_active(irqfd));
> >+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >+ irqfd, irqfd->kvm, irqfd->gsi);
> >
> > list_del_init(&irqfd->list);
> >
> >@@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
> >
> > irqfd->kvm = kvm;
> > irqfd->gsi = gsi;
> >+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >+ irqfd, kvm, gsi);
> > INIT_LIST_HEAD(&irqfd->list);
> > INIT_WORK(&irqfd->inject, irqfd_inject);
> > INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
> >@@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
> > struct _irqfd *irqfd, *tmp;
> > struct eventfd_ctx *eventfd;
> >
> >+ printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
> >+ kvm, gsi);
> > eventfd = eventfd_ctx_fdget(fd);
> > if (IS_ERR(eventfd))
> > return PTR_ERR(eventfd);
> >@@ -305,6 +325,7 @@ void
> > kvm_irqfd_release(struct kvm *kvm)
> > {
> > struct _irqfd *irqfd, *tmp;
> >+ printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
> >
> > spin_lock_irq(&kvm->irqfds.lock);
> >
> >--
> >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
> Hi,
>
> I boot the host with the patched kernel yesterday.
> No crach until now, but two "Eventfd bug detected!" in the log at
> "Mar 9 02:04:31" and "Mar 9 02:15:17"
> You can find part of the log at the following adress:
> http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
>
> It a split file of the kern.log (1,4G), so if you need another part
> of the log, let me know.
>
> Thanks for all.
>
> Regards.
Downloading, it's big :)
What about some 1000 lines before and after Eventfd bug detected! line?
^ permalink raw reply
* Reply.
From: Barr. Tan Chee Lan @ 2011-03-09 12:34 UTC (permalink / raw)
Dear Friend,
I am Barrister Tan Chee Lan, I have contacted you to assist in distributing the
money left behind by my client before it is confiscated or declared unserviceabl
e by the bank where this deposit valued at Six Million, One Hundred Thousand Uni
ted States Dollars (US$6.1 Million). This bank has issued me a notice to contact
the next of kin, or the account will be confiscated.
Best regards,
Tan Chee Lan
Attorney at Law
^ permalink raw reply
* Re: [Patch V2] bonding: fix netpoll in active-backup mode
From: Cong Wang @ 2011-03-09 12:34 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: linux-kernel, Neil Horman, Jay Vosburgh, netdev
In-Reply-To: <20110308212405.GX11864@gospo.rdu.redhat.com>
于 2011年03月09日 05:24, Andy Gospodarek 写道:
> On Tue, Mar 08, 2011 at 05:58:56PM +0800, Amerigo Wang wrote:
>> V2: avoid calling slave_diable_netpoll() with write_lock_bh() held.
>>
>> netconsole doesn't work in active-backup mode, because we don't do anything
>> for nic failover in active-backup mode. We should disable netpoll on the
>> failing slave when it is detected down and enable netpoll when it becomes
>> the active slave.
>>
>> Tested by ifdown the current active slave and ifup it again for several times,
>> netconsole works well.
>>
>> Signed-off-by: WANG Cong<amwang@redhat.com>
>> Cc: Neil Horman<nhorman@tuxdriver.com>
>>
>
> It seems like you are going to a lot of trouble to fix a bug where
> netpoll will not be setup on any interface that is down when enslaved.
> That seems to be the only path that would not have slave->np setup
> properly at enslavement.
>
> Did you ever try just this?
That was my first thought, but I was over-worried about the failing slave.
This way should work too. Mind to send it as a normal patch? :)
Thanks!
^ permalink raw reply
* Re: [Patch V2] bonding: fix netpoll in active-backup mode
From: Cong Wang @ 2011-03-09 12:32 UTC (permalink / raw)
To: Neil Horman; +Cc: linux-kernel, Jay Vosburgh, netdev
In-Reply-To: <20110308134348.GB19072@hmsreliant.think-freely.org>
于 2011年03月08日 21:43, Neil Horman 写道:
> On Tue, Mar 08, 2011 at 05:58:56PM +0800, Amerigo Wang wrote:
>> V2: avoid calling slave_diable_netpoll() with write_lock_bh() held.
>>
>> netconsole doesn't work in active-backup mode, because we don't do anything
>> for nic failover in active-backup mode. We should disable netpoll on the
>> failing slave when it is detected down and enable netpoll when it becomes
>> the active slave.
>>
> You still haven't explained why it needs to be this way. what exactly is the
> shortcomming with leaving netpoll enabled on all slaves, regardless of state?
> It should be sufficient if, during a failover in a mode where the inactive slave
> should not tx data, that you simply clear the slave __LINK_STATE_START bit. That will
> prevent higher layers from sending any queued data without you needing to muck
> about with netpoll state.
>
Oh, actually I thought the struct net_device of the failing interface will be freed,
but that is not true...
So, yeah, I think just setup netpoll in bond_netpoll_setup() should be sufficient.
Thanks.
^ permalink raw reply
* Re: Bug inkvm_set_irq
From: Jean-Philippe Menil @ 2011-03-09 12:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20110308111320.GA8002@redhat.com>
Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
> On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
>> Yes, it's a 2.6.37.2 kernel.
> OK, here's a debugging patch.
> Please run with slab debugging as previously until you see
> 'eventfd bug detected!' in dmesg or until there is a crash.
> It might be also useful to enable timestampts on printk with
> Symbol: PRINTK_TIME [=y]
> │ Type : boolean
> │ Prompt: Show timing information on printks
>
> once you see the error, please upload the
> full dmesg output somewhere to we can track what
> goes on.
>
> Hopefully there won't be an oops this time which
> should make it easier for you to test (no need to
> reboot).
>
>
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index c1f1e3c..3cb679b 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -32,6 +32,7 @@
> #include<linux/eventfd.h>
> #include<linux/kernel.h>
> #include<linux/slab.h>
> +#include<linux/nmi.h>
>
> #include "iodev.h"
>
> @@ -43,6 +44,8 @@
> * --------------------------------------------------------------------
> */
>
> +#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
> +
> struct _irqfd {
> struct kvm *kvm;
> struct eventfd_ctx *eventfd;
> @@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
> {
> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> struct kvm *kvm = irqfd->kvm;
> + if (kvm == KVM_BAD_PTR) {
> + printk(KERN_ERR "Eventfd bug detected!\n");
> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
> + work, irqfd, kvm, irqfd->gsi);
> + trigger_all_cpu_backtrace();
> + return;
> + }
>
> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
> @@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
> struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
> u64 cnt;
>
> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> + work, irqfd, irqfd->kvm, irqfd->gsi);
> /*
> * Synchronize with the wait-queue and unhook ourselves to prevent
> * further events.
> @@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
> * It is now safe to release the object's resources
> */
> eventfd_ctx_put(irqfd->eventfd);
> + printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
> + work, irqfd);
> kfree(irqfd);
> }
>
> @@ -111,6 +125,8 @@ static void
> irqfd_deactivate(struct _irqfd *irqfd)
> {
> BUG_ON(!irqfd_is_active(irqfd));
> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> + irqfd, irqfd->kvm, irqfd->gsi);
>
> list_del_init(&irqfd->list);
>
> @@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
>
> irqfd->kvm = kvm;
> irqfd->gsi = gsi;
> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> + irqfd, kvm, gsi);
> INIT_LIST_HEAD(&irqfd->list);
> INIT_WORK(&irqfd->inject, irqfd_inject);
> INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
> @@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
> struct _irqfd *irqfd, *tmp;
> struct eventfd_ctx *eventfd;
>
> + printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
> + kvm, gsi);
> eventfd = eventfd_ctx_fdget(fd);
> if (IS_ERR(eventfd))
> return PTR_ERR(eventfd);
> @@ -305,6 +325,7 @@ void
> kvm_irqfd_release(struct kvm *kvm)
> {
> struct _irqfd *irqfd, *tmp;
> + printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
>
> spin_lock_irq(&kvm->irqfds.lock);
>
> --
> 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
Hi,
I boot the host with the patched kernel yesterday.
No crach until now, but two "Eventfd bug detected!" in the log at "Mar
9 02:04:31" and "Mar 9 02:15:17"
You can find part of the log at the following adress:
http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
It a split file of the kern.log (1,4G), so if you need another part of
the log, let me know.
Thanks for all.
Regards.
--
Jean-Philippe Menil - Pôle réseau Service IRTS
DSI Université de Nantes
jean-philippe.menil@univ-nantes.fr
Tel : 02.53.48.49.27 - Fax : 02.53.48.49.09
^ permalink raw reply
* [PATCH 8/8] Phonet: kill the ST-Ericsson pipe controller Kconfig
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
This is now a run-time choice so that a single kernel can support both
old and new generation ISI modems. Support for manually enabling the
pipe flow is removed as it did not work properly, does not fit well
with the socket API, and I am not aware of any use at the moment.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
Documentation/networking/phonet.txt | 13 -------------
include/linux/phonet.h | 2 --
net/phonet/Kconfig | 12 ------------
net/phonet/pep.c | 25 -------------------------
4 files changed, 0 insertions(+), 52 deletions(-)
diff --git a/Documentation/networking/phonet.txt b/Documentation/networking/phonet.txt
index 3d12779..8100358 100644
--- a/Documentation/networking/phonet.txt
+++ b/Documentation/networking/phonet.txt
@@ -205,19 +205,6 @@ The pipe protocol provides two socket options at the SOL_PNPIPE level:
socket descriptors that are already connected or being connected.
-Phonet Pipe-controller Implementation
--------------------------------------
-
-Phonet Pipe-controller is enabled by selecting the CONFIG_PHONET_PIPECTRLR
-Kconfig option.
-
-The implementation adds socket options at SOL_PNPIPE level:
-
- PNPIPE_ENABLE accepts one integer value (int). If set to zero, the pipe
- is disabled. If the value is non-zero, the pipe is enabled. If the pipe
- is not (yet) connected, ENOTCONN is error is returned.
-
-
Authors
-------
diff --git a/include/linux/phonet.h b/include/linux/phonet.h
index 32a0965..6fb1384 100644
--- a/include/linux/phonet.h
+++ b/include/linux/phonet.h
@@ -37,8 +37,6 @@
#define PNPIPE_ENCAP 1
#define PNPIPE_IFINDEX 2
#define PNPIPE_HANDLE 3
-#define PNPIPE_ENABLE 4
-/* unused slot */
#define PNADDR_ANY 0
#define PNADDR_BROADCAST 0xFC
diff --git a/net/phonet/Kconfig b/net/phonet/Kconfig
index 0d9b8a2..6ec7d55 100644
--- a/net/phonet/Kconfig
+++ b/net/phonet/Kconfig
@@ -14,15 +14,3 @@ config PHONET
To compile this driver as a module, choose M here: the module
will be called phonet. If unsure, say N.
-
-config PHONET_PIPECTRLR
- bool "Phonet Pipe Controller (EXPERIMENTAL)"
- depends on PHONET && EXPERIMENTAL
- default N
- help
- The Pipe Controller implementation in Phonet stack to support Pipe
- data with Nokia Slim modems like WG2.5 used on ST-Ericsson U8500
- platform.
-
- This option is incompatible with older Nokia modems.
- Say N here unless you really know what you are doing.
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 671effb..68e635f 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -167,15 +167,6 @@ static int pipe_handler_send_created_ind(struct sock *sk)
data, 4, GFP_ATOMIC);
}
-#ifdef CONFIG_PHONET_PIPECTRLR
-static int pipe_handler_enable_pipe(struct sock *sk, int enable)
-{
- u8 id = enable ? PNS_PEP_ENABLE_REQ : PNS_PEP_DISABLE_REQ;
-
- return pipe_handler_request(sk, id, PAD, NULL, 0);
-}
-#endif
-
static int pep_accept_conn(struct sock *sk, struct sk_buff *skb)
{
static const u8 data[20] = {
@@ -968,16 +959,6 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
}
goto out_norel;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNPIPE_ENABLE:
- if ((1 << sk->sk_state) & ~(TCPF_SYN_RECV|TCPF_ESTABLISHED)) {
- err = -ENOTCONN;
- break;
- }
- err = pipe_handler_enable_pipe(sk, val);
- break;
-#endif
-
default:
err = -ENOPROTOOPT;
}
@@ -1013,12 +994,6 @@ static int pep_getsockopt(struct sock *sk, int level, int optname,
return -EINVAL;
break;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNPIPE_ENABLE:
- val = sk->sk_state == TCP_ESTABLISHED;
- break;
-#endif
-
default:
return -ENOPROTOOPT;
}
--
1.7.1
^ permalink raw reply related
* [PATCH 7/8] Phonet: support active connection without pipe controller on modem
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
This provides support for newer ISI modems with no need for the
earlier experimental compile-time alternative choice. With this,
we can now use the same kernel and userspace with both types of
modems.
This also avoids confusing two different and incompatible state
machines, actively connected vs accepted sockets, and adds
connection response error handling (processing "SYN/RST" of sorts).
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
Documentation/networking/phonet.txt | 53 +++++------
net/phonet/pep.c | 172 ++++++++++++++++++++--------------
net/phonet/socket.c | 102 ++++++++-------------
3 files changed, 165 insertions(+), 162 deletions(-)
diff --git a/Documentation/networking/phonet.txt b/Documentation/networking/phonet.txt
index cacac96..3d12779 100644
--- a/Documentation/networking/phonet.txt
+++ b/Documentation/networking/phonet.txt
@@ -154,9 +154,28 @@ connections, one per accept()'d socket.
write(cfd, msg, msglen);
}
-Connections are established between two endpoints by a "third party"
-application. This means that both endpoints are passive; so connect()
-is not possible.
+Connections are traditionally established between two endpoints by a
+"third party" application. This means that both endpoints are passive.
+
+
+As of Linux kernel version 2.6.39, it is also possible to connect
+two endpoints directly, using connect() on the active side. This is
+intended to support the newer Nokia Wireless Modem API, as found in
+e.g. the Nokia Slim Modem in the ST-Ericsson U8500 platform:
+
+ struct sockaddr_spn spn;
+ int fd;
+
+ fd = socket(PF_PHONET, SOCK_SEQPACKET, PN_PROTO_PIPE);
+ memset(&spn, 0, sizeof(spn));
+ spn.spn_family = AF_PHONET;
+ spn.spn_obj = ...;
+ spn.spn_dev = ...;
+ spn.spn_resource = 0xD9;
+ connect(fd, (struct sockaddr *)&spn, sizeof(spn));
+ /* normal I/O here ... */
+ close(fd);
+
WARNING:
When polling a connected pipe socket for writability, there is an
@@ -189,17 +208,8 @@ The pipe protocol provides two socket options at the SOL_PNPIPE level:
Phonet Pipe-controller Implementation
-------------------------------------
-Phonet Pipe-controller is enabled by selecting the CONFIG_PHONET_PIPECTRLR Kconfig
-option. It is useful when communicating with those Nokia Modems which do not
-implement Pipe controller in them e.g. Nokia Slim Modem used in ST-Ericsson
-U8500 platform.
-
-The implementation is based on the Data Connection Establishment Sequence
-depicted in 'Nokia Wireless Modem API - Wireless_modem_user_guide.pdf'
-document.
-
-It allows a phonet sequenced socket (host-pep) to initiate a Pipe connection
-between itself and a remote pipe-end point (e.g. modem).
+Phonet Pipe-controller is enabled by selecting the CONFIG_PHONET_PIPECTRLR
+Kconfig option.
The implementation adds socket options at SOL_PNPIPE level:
@@ -207,21 +217,6 @@ The implementation adds socket options at SOL_PNPIPE level:
is disabled. If the value is non-zero, the pipe is enabled. If the pipe
is not (yet) connected, ENOTCONN is error is returned.
-The implementation also adds socket 'connect'. On calling the 'connect', pipe
-will be created between the source socket and the destination, and the pipe
-state will be set to PIPE_DISABLED.
-
-After a pipe has been created and enabled successfully, the Pipe data can be
-exchanged between the host-pep and remote-pep (modem).
-
-User-space would typically follow below sequence with Pipe controller:-
--socket
--bind
--setsockopt for PNPIPE_PIPE_HANDLE
--connect
--setsockopt for PNPIPE_ENCAP_IP
--setsockopt for PNPIPE_ENABLE
-
Authors
-------
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index abfb795..671effb 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -136,7 +136,6 @@ static int pep_indicate(struct sock *sk, u8 id, u8 code,
#define PAD 0x00
-#ifdef CONFIG_PHONET_PIPECTRLR
static int pipe_handler_request(struct sock *sk, u8 id, u8 code,
const void *data, int len)
{
@@ -168,11 +167,7 @@ static int pipe_handler_send_created_ind(struct sock *sk)
data, 4, GFP_ATOMIC);
}
-static int pipe_handler_send_ind(struct sock *sk, u8 id)
-{
- return pep_indicate(sk, id, PAD, NULL, 0, GFP_ATOMIC);
-}
-
+#ifdef CONFIG_PHONET_PIPECTRLR
static int pipe_handler_enable_pipe(struct sock *sk, int enable)
{
u8 id = enable ? PNS_PEP_ENABLE_REQ : PNS_PEP_DISABLE_REQ;
@@ -376,32 +371,11 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
sk->sk_state_change(sk);
break;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNS_PEP_DISCONNECT_RESP:
- sk->sk_state = TCP_CLOSE;
- break;
-#endif
-
case PNS_PEP_ENABLE_REQ:
/* Wait for PNS_PIPE_(ENABLED|REDIRECTED)_IND */
pep_reply(sk, skb, PN_PIPE_NO_ERROR, NULL, 0, GFP_ATOMIC);
break;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNS_PEP_ENABLE_RESP:
- pipe_handler_send_ind(sk, PNS_PIPE_ENABLED_IND);
-
- if (!pn_flow_safe(pn->tx_fc)) {
- atomic_set(&pn->tx_credits, 1);
- sk->sk_write_space(sk);
- }
- if (sk->sk_state == TCP_ESTABLISHED)
- break; /* Nothing to do */
- sk->sk_state = TCP_ESTABLISHED;
- pipe_grant_credits(sk, GFP_ATOMIC);
- break;
-#endif
-
case PNS_PEP_RESET_REQ:
switch (hdr->state_after_reset) {
case PN_PIPE_DISABLE:
@@ -420,15 +394,6 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
pep_reply(sk, skb, PN_PIPE_NO_ERROR, NULL, 0, GFP_ATOMIC);
break;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNS_PEP_DISABLE_RESP:
- atomic_set(&pn->tx_credits, 0);
- pipe_handler_send_ind(sk, PNS_PIPE_DISABLED_IND);
- sk->sk_state = TCP_SYN_RECV;
- pn->rx_credits = 0;
- break;
-#endif
-
case PNS_PEP_CTRL_REQ:
if (skb_queue_len(&pn->ctrlreq_queue) >= PNPIPE_CTRLREQ_MAX) {
atomic_inc(&sk->sk_drops);
@@ -521,7 +486,6 @@ static void pipe_destruct(struct sock *sk)
skb_queue_purge(&pn->ctrlreq_queue);
}
-#ifdef CONFIG_PHONET_PIPECTRLR
static u8 pipe_negotiate_fc(const u8 *fcs, unsigned n)
{
unsigned i;
@@ -546,6 +510,8 @@ static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
return -EINVAL;
hdr = pnp_hdr(skb);
+ if (hdr->error_code != PN_PIPE_NO_ERROR)
+ return -ECONNREFUSED;
/* Parse sub-blocks */
n_sb = hdr->data[4];
@@ -573,14 +539,74 @@ static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
n_sb--;
}
- sk->sk_state = TCP_SYN_RECV;
- sk->sk_backlog_rcv = pipe_do_rcv;
- pn->rx_credits = 0;
- sk->sk_state_change(sk);
-
return pipe_handler_send_created_ind(sk);
}
-#endif
+
+/* Queue an skb to an actively connected sock.
+ * Socket lock must be held. */
+static int pipe_handler_do_rcv(struct sock *sk, struct sk_buff *skb)
+{
+ struct pep_sock *pn = pep_sk(sk);
+ struct pnpipehdr *hdr = pnp_hdr(skb);
+ int err = NET_RX_SUCCESS;
+
+ switch (hdr->message_id) {
+ case PNS_PIPE_ALIGNED_DATA:
+ __skb_pull(skb, 1);
+ /* fall through */
+ case PNS_PIPE_DATA:
+ __skb_pull(skb, 3); /* Pipe data header */
+ if (!pn_flow_safe(pn->rx_fc)) {
+ err = sock_queue_rcv_skb(sk, skb);
+ if (!err)
+ return NET_RX_SUCCESS;
+ err = NET_RX_DROP;
+ break;
+ }
+
+ if (pn->rx_credits == 0) {
+ atomic_inc(&sk->sk_drops);
+ err = NET_RX_DROP;
+ break;
+ }
+ pn->rx_credits--;
+ skb->dev = NULL;
+ skb_set_owner_r(skb, sk);
+ err = skb->len;
+ skb_queue_tail(&sk->sk_receive_queue, skb);
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk->sk_data_ready(sk, err);
+ return NET_RX_SUCCESS;
+
+ case PNS_PEP_CONNECT_RESP:
+ if (sk->sk_state != TCP_SYN_SENT)
+ break;
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk->sk_state_change(sk);
+ if (pep_connresp_rcv(sk, skb)) {
+ sk->sk_state = TCP_CLOSE_WAIT;
+ break;
+ }
+
+ sk->sk_state = TCP_ESTABLISHED;
+ if (!pn_flow_safe(pn->tx_fc)) {
+ atomic_set(&pn->tx_credits, 1);
+ sk->sk_write_space(sk);
+ }
+ pipe_grant_credits(sk, GFP_ATOMIC);
+ break;
+
+ case PNS_PEP_DISCONNECT_RESP:
+ /* sock should already be dead, nothing to do */
+ break;
+
+ case PNS_PEP_STATUS_IND:
+ pipe_rcv_status(sk, skb);
+ break;
+ }
+ kfree_skb(skb);
+ return err;
+}
/* Listening sock must be locked */
static struct sock *pep_find_pipe(const struct hlist_head *hlist,
@@ -649,12 +675,6 @@ static int pep_do_rcv(struct sock *sk, struct sk_buff *skb)
sk->sk_data_ready(sk, 0);
return NET_RX_SUCCESS;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNS_PEP_CONNECT_RESP:
- pep_connresp_rcv(sk, skb);
- break;
-#endif
-
case PNS_PEP_DISCONNECT_REQ:
pep_reply(sk, skb, PN_PIPE_NO_ERROR, NULL, 0, GFP_ATOMIC);
break;
@@ -667,15 +687,19 @@ static int pep_do_rcv(struct sock *sk, struct sk_buff *skb)
case PNS_PEP_ENABLE_REQ:
case PNS_PEP_DISABLE_REQ:
/* invalid handle is not even allowed here! */
- default:
break;
+
+ default:
+ if ((1 << sk->sk_state)
+ & ~(TCPF_CLOSE|TCPF_LISTEN|TCPF_CLOSE_WAIT))
+ /* actively connected socket */
+ return pipe_handler_do_rcv(sk, skb);
}
drop:
kfree_skb(skb);
return NET_RX_SUCCESS;
}
-#ifndef CONFIG_PHONET_PIPECTRLR
static int pipe_do_remove(struct sock *sk)
{
struct pep_sock *pn = pep_sk(sk);
@@ -693,7 +717,6 @@ static int pipe_do_remove(struct sock *sk)
ph->data[0] = PAD;
return pn_skb_send(sk, skb, NULL);
}
-#endif
/* associated socket ceases to exist */
static void pep_sock_close(struct sock *sk, long timeout)
@@ -706,13 +729,12 @@ static void pep_sock_close(struct sock *sk, long timeout)
lock_sock(sk);
if ((1 << sk->sk_state) & (TCPF_SYN_RECV|TCPF_ESTABLISHED)) {
-#ifndef CONFIG_PHONET_PIPECTRLR
- /* Forcefully remove dangling Phonet pipe */
- pipe_do_remove(sk);
-#else
- /* send pep disconnect request */
- pipe_handler_request(sk, PNS_PEP_DISCONNECT_REQ, PAD, NULL, 0);
-#endif
+ if (sk->sk_backlog_rcv == pipe_do_rcv)
+ /* Forcefully remove dangling Phonet pipe */
+ pipe_do_remove(sk);
+ else
+ pipe_handler_request(sk, PNS_PEP_DISCONNECT_REQ, PAD,
+ NULL, 0);
}
sk->sk_state = TCP_CLOSE;
@@ -844,20 +866,22 @@ drop:
return newsk;
}
-#ifdef CONFIG_PHONET_PIPECTRLR
static int pep_sock_connect(struct sock *sk, struct sockaddr *addr, int len)
{
struct pep_sock *pn = pep_sk(sk);
- const struct sockaddr_pn *spn = (struct sockaddr_pn *)addr;
+ int err;
u8 data[4] = { 0 /* sub-blocks */, PAD, PAD, PAD };
- pn->pn_sk.dobject = pn_sockaddr_get_object(spn);
- pn->pn_sk.resource = pn_sockaddr_get_resource(spn);
pn->pipe_handle = 1; /* anything but INVALID_HANDLE */
- return pipe_handler_request(sk, PNS_PEP_CONNECT_REQ,
- PN_PIPE_DISABLE, data, 4);
+ err = pipe_handler_request(sk, PNS_PEP_CONNECT_REQ,
+ PN_PIPE_ENABLE, data, 4);
+ if (err) {
+ pn->pipe_handle = PN_PIPE_INVALID_HANDLE;
+ return err;
+ }
+ sk->sk_state = TCP_SYN_SENT;
+ return 0;
}
-#endif
static int pep_ioctl(struct sock *sk, int cmd, unsigned long arg)
{
@@ -890,8 +914,16 @@ static int pep_init(struct sock *sk)
sk->sk_destruct = pipe_destruct;
INIT_HLIST_HEAD(&pn->hlist);
+ pn->listener = NULL;
skb_queue_head_init(&pn->ctrlreq_queue);
+ atomic_set(&pn->tx_credits, 0);
+ pn->ifindex = 0;
+ pn->peer_type = 0;
pn->pipe_handle = PN_PIPE_INVALID_HANDLE;
+ pn->rx_credits = 0;
+ pn->rx_fc = pn->tx_fc = PN_LEGACY_FLOW_CONTROL;
+ pn->init_enable = 1;
+ pn->aligned = 0;
return 0;
}
@@ -1219,9 +1251,9 @@ static void pep_sock_unhash(struct sock *sk)
lock_sock(sk);
-#ifndef CONFIG_PHONET_PIPECTRLR
- if ((1 << sk->sk_state) & ~(TCPF_CLOSE|TCPF_LISTEN)) {
+ if (pn->listener != NULL) {
skparent = pn->listener;
+ pn->listener = NULL;
release_sock(sk);
pn = pep_sk(skparent);
@@ -1229,7 +1261,7 @@ static void pep_sock_unhash(struct sock *sk)
sk_del_node_init(sk);
sk = skparent;
}
-#endif
+
/* Unhash a listening sock only when it is closed
* and all of its active connected pipes are closed. */
if (hlist_empty(&pn->hlist))
@@ -1243,9 +1275,7 @@ static void pep_sock_unhash(struct sock *sk)
static struct proto pep_proto = {
.close = pep_sock_close,
.accept = pep_sock_accept,
-#ifdef CONFIG_PHONET_PIPECTRLR
.connect = pep_sock_connect,
-#endif
.ioctl = pep_ioctl,
.init = pep_init,
.setsockopt = pep_setsockopt,
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 1eccfc3..b1adafa 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -225,15 +225,18 @@ static int pn_socket_autobind(struct socket *sock)
return 0; /* socket was already bound */
}
-#ifdef CONFIG_PHONET_PIPECTRLR
static int pn_socket_connect(struct socket *sock, struct sockaddr *addr,
int len, int flags)
{
struct sock *sk = sock->sk;
+ struct pn_sock *pn = pn_sk(sk);
struct sockaddr_pn *spn = (struct sockaddr_pn *)addr;
- long timeo;
+ struct task_struct *tsk = current;
+ long timeo = sock_rcvtimeo(sk, flags & O_NONBLOCK);
int err;
+ if (pn_socket_autobind(sock))
+ return -ENOBUFS;
if (len < sizeof(struct sockaddr_pn))
return -EINVAL;
if (spn->spn_family != AF_PHONET)
@@ -243,82 +246,61 @@ static int pn_socket_connect(struct socket *sock, struct sockaddr *addr,
switch (sock->state) {
case SS_UNCONNECTED:
- sk->sk_state = TCP_CLOSE;
- break;
- case SS_CONNECTING:
- switch (sk->sk_state) {
- case TCP_SYN_RECV:
- sock->state = SS_CONNECTED;
- err = -EISCONN;
- goto out;
- case TCP_CLOSE:
- err = -EALREADY;
- if (flags & O_NONBLOCK)
- goto out;
- goto wait_connect;
- }
- break;
- case SS_CONNECTED:
- switch (sk->sk_state) {
- case TCP_SYN_RECV:
+ if (sk->sk_state != TCP_CLOSE) {
err = -EISCONN;
goto out;
- case TCP_CLOSE:
- sock->state = SS_UNCONNECTED;
- break;
}
break;
- case SS_DISCONNECTING:
- case SS_FREE:
- break;
+ case SS_CONNECTING:
+ err = -EALREADY;
+ goto out;
+ default:
+ err = -EISCONN;
+ goto out;
}
- sk->sk_state = TCP_CLOSE;
- sk_stream_kill_queues(sk);
+ pn->dobject = pn_sockaddr_get_object(spn);
+ pn->resource = pn_sockaddr_get_resource(spn);
sock->state = SS_CONNECTING;
+
err = sk->sk_prot->connect(sk, addr, len);
- if (err < 0) {
+ if (err) {
sock->state = SS_UNCONNECTED;
- sk->sk_state = TCP_CLOSE;
+ pn->dobject = 0;
goto out;
}
- err = -EINPROGRESS;
-wait_connect:
- if (sk->sk_state != TCP_SYN_RECV && (flags & O_NONBLOCK))
- goto out;
-
- timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
- release_sock(sk);
+ while (sk->sk_state == TCP_SYN_SENT) {
+ DEFINE_WAIT(wait);
- err = -ERESTARTSYS;
- timeo = wait_event_interruptible_timeout(*sk_sleep(sk),
- sk->sk_state != TCP_CLOSE,
- timeo);
-
- lock_sock(sk);
- if (timeo < 0)
- goto out; /* -ERESTARTSYS */
-
- err = -ETIMEDOUT;
- if (timeo == 0 && sk->sk_state != TCP_SYN_RECV)
- goto out;
+ if (!timeo) {
+ err = -EINPROGRESS;
+ goto out;
+ }
+ if (signal_pending(tsk)) {
+ err = sock_intr_errno(timeo);
+ goto out;
+ }
- if (sk->sk_state != TCP_SYN_RECV) {
- sock->state = SS_UNCONNECTED;
- err = sock_error(sk);
- if (!err)
- err = -ECONNREFUSED;
- goto out;
+ prepare_to_wait_exclusive(sk_sleep(sk), &wait,
+ TASK_INTERRUPTIBLE);
+ release_sock(sk);
+ timeo = schedule_timeout(timeo);
+ lock_sock(sk);
+ finish_wait(sk_sleep(sk), &wait);
}
- sock->state = SS_CONNECTED;
- err = 0;
+ if ((1 << sk->sk_state) & (TCPF_SYN_RECV|TCPF_ESTABLISHED))
+ err = 0;
+ else if (sk->sk_state == TCP_CLOSE_WAIT)
+ err = -ECONNRESET;
+ else
+ err = -ECONNREFUSED;
+ sock->state = err ? SS_UNCONNECTED : SS_CONNECTED;
out:
release_sock(sk);
return err;
}
-#endif
static int pn_socket_accept(struct socket *sock, struct socket *newsock,
int flags)
@@ -486,11 +468,7 @@ const struct proto_ops phonet_stream_ops = {
.owner = THIS_MODULE,
.release = pn_socket_release,
.bind = pn_socket_bind,
-#ifdef CONFIG_PHONET_PIPECTRLR
.connect = pn_socket_connect,
-#else
- .connect = sock_no_connect,
-#endif
.socketpair = sock_no_socketpair,
.accept = pn_socket_accept,
.getname = pn_socket_getname,
--
1.7.1
^ permalink raw reply related
* [PATCH 6/8] Phonet: provide pipe socket option to retrieve the pipe identifier
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
User-space sometimes needs this information. In particular, the GPRS
context or the AT commands pipe setups may use the pipe handle as a
reference.
This removes the settable pipe handle with CONFIG_PHONET_PIPECTRLR.
It did not handle error cases correctly. Furthermore, the kernel
*could* implement a smart scheme for allocating handles (if ever
needed), but userspace really cannot.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
Documentation/networking/phonet.txt | 7 ++++---
include/linux/phonet.h | 2 +-
net/phonet/pep.c | 15 +++++++--------
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/Documentation/networking/phonet.txt b/Documentation/networking/phonet.txt
index 24ad2ad..cacac96 100644
--- a/Documentation/networking/phonet.txt
+++ b/Documentation/networking/phonet.txt
@@ -181,6 +181,10 @@ The pipe protocol provides two socket options at the SOL_PNPIPE level:
interface index of the network interface created by PNPIPE_ENCAP,
or zero if encapsulation is off.
+ PNPIPE_HANDLE is a read-only integer value. It contains the underlying
+ identifier ("pipe handle") of the pipe. This is only defined for
+ socket descriptors that are already connected or being connected.
+
Phonet Pipe-controller Implementation
-------------------------------------
@@ -199,9 +203,6 @@ between itself and a remote pipe-end point (e.g. modem).
The implementation adds socket options at SOL_PNPIPE level:
- PNPIPE_PIPE_HANDLE
- It accepts an integer argument for setting value of pipe handle.
-
PNPIPE_ENABLE accepts one integer value (int). If set to zero, the pipe
is disabled. If the value is non-zero, the pipe is enabled. If the pipe
is not (yet) connected, ENOTCONN is error is returned.
diff --git a/include/linux/phonet.h b/include/linux/phonet.h
index 26c8df7..32a0965 100644
--- a/include/linux/phonet.h
+++ b/include/linux/phonet.h
@@ -36,7 +36,7 @@
/* Socket options for SOL_PNPIPE level */
#define PNPIPE_ENCAP 1
#define PNPIPE_IFINDEX 2
-#define PNPIPE_PIPE_HANDLE 3
+#define PNPIPE_HANDLE 3
#define PNPIPE_ENABLE 4
/* unused slot */
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index c0fab4c..abfb795 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -853,6 +853,7 @@ static int pep_sock_connect(struct sock *sk, struct sockaddr *addr, int len)
pn->pn_sk.dobject = pn_sockaddr_get_object(spn);
pn->pn_sk.resource = pn_sockaddr_get_resource(spn);
+ pn->pipe_handle = 1; /* anything but INVALID_HANDLE */
return pipe_handler_request(sk, PNS_PEP_CONNECT_REQ,
PN_PIPE_DISABLE, data, 4);
}
@@ -909,14 +910,6 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
lock_sock(sk);
switch (optname) {
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNPIPE_PIPE_HANDLE:
- if (val) {
- pn->pipe_handle = val;
- break;
- }
-#endif
-
case PNPIPE_ENCAP:
if (val && val != PNPIPE_ENCAP_IP) {
err = -EINVAL;
@@ -982,6 +975,12 @@ static int pep_getsockopt(struct sock *sk, int level, int optname,
val = pn->ifindex;
break;
+ case PNPIPE_HANDLE:
+ val = pn->pipe_handle;
+ if (val == PN_PIPE_INVALID_HANDLE)
+ return -EINVAL;
+ break;
+
#ifdef CONFIG_PHONET_PIPECTRLR
case PNPIPE_ENABLE:
val = sk->sk_state == TCP_ESTABLISHED;
--
1.7.1
^ permalink raw reply related
* [PATCH 5/8] Phonet: allocate sock from accept syscall rather than soft IRQ
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
This moves most of the accept logic to process context like other
socket stacks do. Then we can use a few more common socket helpers
and simplify a bit.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
include/net/phonet/pep.h | 1 -
net/phonet/pep.c | 284 +++++++++++++++++++---------------------------
net/phonet/socket.c | 10 +-
3 files changed, 121 insertions(+), 174 deletions(-)
diff --git a/include/net/phonet/pep.h b/include/net/phonet/pep.h
index 38eed1b..b669fe6 100644
--- a/include/net/phonet/pep.h
+++ b/include/net/phonet/pep.h
@@ -28,7 +28,6 @@ struct pep_sock {
/* XXX: union-ify listening vs connected stuff ? */
/* Listening socket stuff: */
- struct hlist_head ackq;
struct hlist_head hlist;
/* Connected socket stuff: */
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 610794a..c0fab4c 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -42,7 +42,7 @@
* TCP_ESTABLISHED connected pipe in enabled state
*
* pep_sock locking:
- * - sk_state, ackq, hlist: sock lock needed
+ * - sk_state, hlist: sock lock needed
* - listener: read only
* - pipe_handle: read only
*/
@@ -202,11 +202,12 @@ static int pep_accept_conn(struct sock *sk, struct sk_buff *skb)
GFP_KERNEL);
}
-static int pep_reject_conn(struct sock *sk, struct sk_buff *skb, u8 code)
+static int pep_reject_conn(struct sock *sk, struct sk_buff *skb, u8 code,
+ gfp_t priority)
{
static const u8 data[4] = { PAD, PAD, PAD, 0 /* sub-blocks */ };
WARN_ON(code == PN_PIPE_NO_ERROR);
- return pep_reply(sk, skb, code, data, sizeof(data), GFP_ATOMIC);
+ return pep_reply(sk, skb, code, data, sizeof(data), priority);
}
/* Control requests are not sent by the pipe service and have a specific
@@ -365,7 +366,7 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
switch (hdr->message_id) {
case PNS_PEP_CONNECT_REQ:
- pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE);
+ pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE, GFP_ATOMIC);
break;
case PNS_PEP_DISCONNECT_REQ:
@@ -574,7 +575,6 @@ static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
sk->sk_state = TCP_SYN_RECV;
sk->sk_backlog_rcv = pipe_do_rcv;
- sk->sk_destruct = pipe_destruct;
pn->rx_credits = 0;
sk->sk_state_change(sk);
@@ -582,96 +582,6 @@ static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
}
#endif
-static int pep_connreq_rcv(struct sock *sk, struct sk_buff *skb)
-{
- struct sock *newsk;
- struct pep_sock *newpn, *pn = pep_sk(sk);
- struct pnpipehdr *hdr;
- struct sockaddr_pn dst, src;
- u16 peer_type;
- u8 pipe_handle, enabled, n_sb;
- u8 aligned = 0;
-
- if (!pskb_pull(skb, sizeof(*hdr) + 4))
- return -EINVAL;
-
- hdr = pnp_hdr(skb);
- pipe_handle = hdr->pipe_handle;
- switch (hdr->state_after_connect) {
- case PN_PIPE_DISABLE:
- enabled = 0;
- break;
- case PN_PIPE_ENABLE:
- enabled = 1;
- break;
- default:
- pep_reject_conn(sk, skb, PN_PIPE_ERR_INVALID_PARAM);
- return -EINVAL;
- }
- peer_type = hdr->other_pep_type << 8;
-
- /* Parse sub-blocks (options) */
- n_sb = hdr->data[4];
- while (n_sb > 0) {
- u8 type, buf[1], len = sizeof(buf);
- const u8 *data = pep_get_sb(skb, &type, &len, buf);
-
- if (data == NULL)
- return -EINVAL;
- switch (type) {
- case PN_PIPE_SB_CONNECT_REQ_PEP_SUB_TYPE:
- if (len < 1)
- return -EINVAL;
- peer_type = (peer_type & 0xff00) | data[0];
- break;
- case PN_PIPE_SB_ALIGNED_DATA:
- aligned = data[0] != 0;
- break;
- }
- n_sb--;
- }
-
- skb = skb_clone(skb, GFP_ATOMIC);
- if (!skb)
- return -ENOMEM;
-
- /* Create a new to-be-accepted sock */
- newsk = sk_alloc(sock_net(sk), PF_PHONET, GFP_ATOMIC, sk->sk_prot);
- if (!newsk) {
- kfree_skb(skb);
- return -ENOMEM;
- }
- sock_init_data(NULL, newsk);
- newsk->sk_state = TCP_SYN_RECV;
- newsk->sk_backlog_rcv = pipe_do_rcv;
- newsk->sk_protocol = sk->sk_protocol;
- newsk->sk_destruct = pipe_destruct;
-
- newpn = pep_sk(newsk);
- pn_skb_get_dst_sockaddr(skb, &dst);
- pn_skb_get_src_sockaddr(skb, &src);
- newpn->pn_sk.sobject = pn_sockaddr_get_object(&dst);
- newpn->pn_sk.dobject = pn_sockaddr_get_object(&src);
- newpn->pn_sk.resource = pn_sockaddr_get_resource(&dst);
- skb_queue_head_init(&newpn->ctrlreq_queue);
- newpn->pipe_handle = pipe_handle;
- atomic_set(&newpn->tx_credits, 0);
- newpn->peer_type = peer_type;
- newpn->rx_credits = 0;
- newpn->rx_fc = newpn->tx_fc = PN_LEGACY_FLOW_CONTROL;
- newpn->init_enable = enabled;
- newpn->aligned = aligned;
-
- BUG_ON(!skb_queue_empty(&newsk->sk_receive_queue));
- skb_queue_head(&newsk->sk_receive_queue, skb);
- if (!sock_flag(sk, SOCK_DEAD))
- sk->sk_data_ready(sk, 0);
-
- sk_acceptq_added(sk);
- sk_add_node(newsk, &pn->ackq);
- return 0;
-}
-
/* Listening sock must be locked */
static struct sock *pep_find_pipe(const struct hlist_head *hlist,
const struct sockaddr_pn *dst,
@@ -726,22 +636,18 @@ static int pep_do_rcv(struct sock *sk, struct sk_buff *skb)
if (sknode)
return sk_receive_skb(sknode, skb, 1);
- /* Look for a pipe handle pending accept */
- sknode = pep_find_pipe(&pn->ackq, &dst, pipe_handle);
- if (sknode) {
- sock_put(sknode);
- if (net_ratelimit())
- printk(KERN_WARNING"Phonet unconnected PEP ignored");
- goto drop;
- }
-
switch (hdr->message_id) {
case PNS_PEP_CONNECT_REQ:
- if (sk->sk_state == TCP_LISTEN && !sk_acceptq_is_full(sk))
- pep_connreq_rcv(sk, skb);
- else
- pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE);
- break;
+ if (sk->sk_state != TCP_LISTEN || sk_acceptq_is_full(sk)) {
+ pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE,
+ GFP_ATOMIC);
+ break;
+ }
+ skb_queue_head(&sk->sk_receive_queue, skb);
+ sk_acceptq_added(sk);
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk->sk_data_ready(sk, 0);
+ return NET_RX_SUCCESS;
#ifdef CONFIG_PHONET_PIPECTRLR
case PNS_PEP_CONNECT_RESP:
@@ -799,24 +705,16 @@ static void pep_sock_close(struct sock *sk, long timeout)
sk_common_release(sk);
lock_sock(sk);
- if (sk->sk_state == TCP_LISTEN) {
- /* Destroy the listen queue */
- struct sock *sknode;
- struct hlist_node *p, *n;
-
- sk_for_each_safe(sknode, p, n, &pn->ackq)
- sk_del_node_init(sknode);
- sk->sk_state = TCP_CLOSE;
- } else if ((1 << sk->sk_state) & (TCPF_SYN_RECV|TCPF_ESTABLISHED)) {
+ if ((1 << sk->sk_state) & (TCPF_SYN_RECV|TCPF_ESTABLISHED)) {
#ifndef CONFIG_PHONET_PIPECTRLR
/* Forcefully remove dangling Phonet pipe */
pipe_do_remove(sk);
#else
/* send pep disconnect request */
pipe_handler_request(sk, PNS_PEP_DISCONNECT_REQ, PAD, NULL, 0);
- sk->sk_state = TCP_CLOSE;
#endif
}
+ sk->sk_state = TCP_CLOSE;
ifindex = pn->ifindex;
pn->ifindex = 0;
@@ -827,69 +725,121 @@ static void pep_sock_close(struct sock *sk, long timeout)
sock_put(sk);
}
-static int pep_wait_connreq(struct sock *sk, int noblock)
+static struct sock *pep_sock_accept(struct sock *sk, int flags, int *errp)
{
- struct task_struct *tsk = current;
- struct pep_sock *pn = pep_sk(sk);
- long timeo = sock_rcvtimeo(sk, noblock);
-
- for (;;) {
- DEFINE_WAIT(wait);
+ struct pep_sock *pn = pep_sk(sk), *newpn;
+ struct sock *newsk = NULL;
+ struct sk_buff *skb;
+ struct pnpipehdr *hdr;
+ struct sockaddr_pn dst, src;
+ int err;
+ u16 peer_type;
+ u8 pipe_handle, enabled, n_sb;
+ u8 aligned = 0;
- if (sk->sk_state != TCP_LISTEN)
- return -EINVAL;
- if (!hlist_empty(&pn->ackq))
- break;
- if (!timeo)
- return -EWOULDBLOCK;
- if (signal_pending(tsk))
- return sock_intr_errno(timeo);
+ skb = skb_recv_datagram(sk, 0, flags & O_NONBLOCK, errp);
+ if (!skb)
+ return NULL;
- prepare_to_wait_exclusive(sk_sleep(sk), &wait,
- TASK_INTERRUPTIBLE);
- release_sock(sk);
- timeo = schedule_timeout(timeo);
- lock_sock(sk);
- finish_wait(sk_sleep(sk), &wait);
+ lock_sock(sk);
+ if (sk->sk_state != TCP_LISTEN) {
+ err = -EINVAL;
+ goto drop;
}
+ sk_acceptq_removed(sk);
- return 0;
-}
+ err = -EPROTO;
+ if (!pskb_may_pull(skb, sizeof(*hdr) + 4))
+ goto drop;
-static struct sock *pep_sock_accept(struct sock *sk, int flags, int *errp)
-{
- struct pep_sock *pn = pep_sk(sk);
- struct sock *newsk = NULL;
- struct sk_buff *oskb;
- int err;
+ hdr = pnp_hdr(skb);
+ pipe_handle = hdr->pipe_handle;
+ switch (hdr->state_after_connect) {
+ case PN_PIPE_DISABLE:
+ enabled = 0;
+ break;
+ case PN_PIPE_ENABLE:
+ enabled = 1;
+ break;
+ default:
+ pep_reject_conn(sk, skb, PN_PIPE_ERR_INVALID_PARAM,
+ GFP_KERNEL);
+ goto drop;
+ }
+ peer_type = hdr->other_pep_type << 8;
- lock_sock(sk);
- err = pep_wait_connreq(sk, flags & O_NONBLOCK);
- if (err)
- goto out;
+ /* Parse sub-blocks (options) */
+ n_sb = hdr->data[4];
+ while (n_sb > 0) {
+ u8 type, buf[1], len = sizeof(buf);
+ const u8 *data = pep_get_sb(skb, &type, &len, buf);
- newsk = __sk_head(&pn->ackq);
+ if (data == NULL)
+ goto drop;
+ switch (type) {
+ case PN_PIPE_SB_CONNECT_REQ_PEP_SUB_TYPE:
+ if (len < 1)
+ goto drop;
+ peer_type = (peer_type & 0xff00) | data[0];
+ break;
+ case PN_PIPE_SB_ALIGNED_DATA:
+ aligned = data[0] != 0;
+ break;
+ }
+ n_sb--;
+ }
- oskb = skb_dequeue(&newsk->sk_receive_queue);
- err = pep_accept_conn(newsk, oskb);
- if (err) {
- skb_queue_head(&newsk->sk_receive_queue, oskb);
+ /* Check for duplicate pipe handle */
+ newsk = pep_find_pipe(&pn->hlist, &dst, pipe_handle);
+ if (unlikely(newsk)) {
+ __sock_put(newsk);
newsk = NULL;
- goto out;
+ pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE, GFP_KERNEL);
+ goto drop;
+ }
+
+ /* Create a new to-be-accepted sock */
+ newsk = sk_alloc(sock_net(sk), PF_PHONET, GFP_KERNEL, sk->sk_prot);
+ if (!newsk) {
+ pep_reject_conn(sk, skb, PN_PIPE_ERR_OVERLOAD, GFP_KERNEL);
+ err = -ENOBUFS;
+ goto drop;
}
- kfree_skb(oskb);
+ sock_init_data(NULL, newsk);
+ newsk->sk_state = TCP_SYN_RECV;
+ newsk->sk_backlog_rcv = pipe_do_rcv;
+ newsk->sk_protocol = sk->sk_protocol;
+ newsk->sk_destruct = pipe_destruct;
+
+ newpn = pep_sk(newsk);
+ pn_skb_get_dst_sockaddr(skb, &dst);
+ pn_skb_get_src_sockaddr(skb, &src);
+ newpn->pn_sk.sobject = pn_sockaddr_get_object(&dst);
+ newpn->pn_sk.dobject = pn_sockaddr_get_object(&src);
+ newpn->pn_sk.resource = pn_sockaddr_get_resource(&dst);
sock_hold(sk);
- pep_sk(newsk)->listener = sk;
+ newpn->listener = sk;
+ skb_queue_head_init(&newpn->ctrlreq_queue);
+ newpn->pipe_handle = pipe_handle;
+ atomic_set(&newpn->tx_credits, 0);
+ newpn->ifindex = 0;
+ newpn->peer_type = peer_type;
+ newpn->rx_credits = 0;
+ newpn->rx_fc = newpn->tx_fc = PN_LEGACY_FLOW_CONTROL;
+ newpn->init_enable = enabled;
+ newpn->aligned = aligned;
- sock_hold(newsk);
- sk_del_node_init(newsk);
- sk_acceptq_removed(sk);
+ err = pep_accept_conn(newsk, skb);
+ if (err) {
+ sock_put(newsk);
+ newsk = NULL;
+ goto drop;
+ }
sk_add_node(newsk, &pn->hlist);
- __sock_put(newsk);
-
-out:
+drop:
release_sock(sk);
+ kfree_skb(skb);
*errp = err;
return newsk;
}
@@ -937,7 +887,7 @@ static int pep_init(struct sock *sk)
{
struct pep_sock *pn = pep_sk(sk);
- INIT_HLIST_HEAD(&pn->ackq);
+ sk->sk_destruct = pipe_destruct;
INIT_HLIST_HEAD(&pn->hlist);
skb_queue_head_init(&pn->ctrlreq_queue);
pn->pipe_handle = PN_PIPE_INVALID_HANDLE;
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 65a0333..1eccfc3 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -327,6 +327,9 @@ static int pn_socket_accept(struct socket *sock, struct socket *newsock,
struct sock *newsk;
int err;
+ if (unlikely(sk->sk_state != TCP_LISTEN))
+ return -EINVAL;
+
newsk = sk->sk_prot->accept(sk, flags, &err);
if (!newsk)
return err;
@@ -363,13 +366,8 @@ static unsigned int pn_socket_poll(struct file *file, struct socket *sock,
poll_wait(file, sk_sleep(sk), wait);
- switch (sk->sk_state) {
- case TCP_LISTEN:
- return hlist_empty(&pn->ackq) ? 0 : POLLIN;
- case TCP_CLOSE:
+ if (sk->sk_state == TCP_CLOSE)
return POLLERR;
- }
-
if (!skb_queue_empty(&sk->sk_receive_queue))
mask |= POLLIN | POLLRDNORM;
if (!skb_queue_empty(&pn->ctrlreq_queue))
--
1.7.1
^ permalink raw reply related
* [PATCH 4/8] Phonet: factor common code to send control messages
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
With the addition of the pipe controller, there is now quite a bit
of repetitive code for small signaling messages. Lets factor it.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
net/phonet/pep.c | 225 ++++++++++++++++++------------------------------------
1 files changed, 73 insertions(+), 152 deletions(-)
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 40952c7..610794a 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -77,24 +77,34 @@ static unsigned char *pep_get_sb(struct sk_buff *skb, u8 *ptype, u8 *plen,
return data;
}
-static int pep_reply(struct sock *sk, struct sk_buff *oskb,
- u8 code, const void *data, int len, gfp_t priority)
+static struct sk_buff *pep_alloc_skb(struct sock *sk, const void *payload,
+ int len, gfp_t priority)
+{
+ struct sk_buff *skb = alloc_skb(MAX_PNPIPE_HEADER + len, priority);
+ if (!skb)
+ return NULL;
+ skb_set_owner_w(skb, sk);
+
+ skb_reserve(skb, MAX_PNPIPE_HEADER);
+ __skb_put(skb, len);
+ skb_copy_to_linear_data(skb, payload, len);
+ __skb_push(skb, sizeof(struct pnpipehdr));
+ skb_reset_transport_header(skb);
+ return skb;
+}
+
+static int pep_reply(struct sock *sk, struct sk_buff *oskb, u8 code,
+ const void *data, int len, gfp_t priority)
{
const struct pnpipehdr *oph = pnp_hdr(oskb);
struct pnpipehdr *ph;
struct sk_buff *skb;
struct sockaddr_pn peer;
- skb = alloc_skb(MAX_PNPIPE_HEADER + len, priority);
+ skb = pep_alloc_skb(sk, data, len, priority);
if (!skb)
return -ENOMEM;
- skb_set_owner_w(skb, sk);
- skb_reserve(skb, MAX_PNPIPE_HEADER);
- __skb_put(skb, len);
- skb_copy_to_linear_data(skb, data, len);
- __skb_push(skb, sizeof(*ph));
- skb_reset_transport_header(skb);
ph = pnp_hdr(skb);
ph->utid = oph->utid;
ph->message_id = oph->message_id + 1; /* REQ -> RESP */
@@ -105,135 +115,69 @@ static int pep_reply(struct sock *sk, struct sk_buff *oskb,
return pn_skb_send(sk, skb, &peer);
}
-#define PAD 0x00
-
-#ifdef CONFIG_PHONET_PIPECTRLR
-static int pipe_handler_send_req(struct sock *sk, u8 msg_id, gfp_t priority)
+static int pep_indicate(struct sock *sk, u8 id, u8 code,
+ const void *data, int len, gfp_t priority)
{
- int len;
+ struct pep_sock *pn = pep_sk(sk);
struct pnpipehdr *ph;
struct sk_buff *skb;
- struct pep_sock *pn = pep_sk(sk);
-
- static const u8 data[4] = {
- PAD, PAD, PAD, PAD,
- };
- switch (msg_id) {
- case PNS_PEP_CONNECT_REQ:
- len = sizeof(data);
- break;
-
- case PNS_PEP_DISCONNECT_REQ:
- case PNS_PEP_ENABLE_REQ:
- case PNS_PEP_DISABLE_REQ:
- len = 0;
- break;
-
- default:
- return -EINVAL;
- }
-
- skb = alloc_skb(MAX_PNPIPE_HEADER + len, priority);
+ skb = pep_alloc_skb(sk, data, len, priority);
if (!skb)
return -ENOMEM;
- skb_set_owner_w(skb, sk);
- skb_reserve(skb, MAX_PNPIPE_HEADER);
- if (len) {
- __skb_put(skb, len);
- skb_copy_to_linear_data(skb, data, len);
- }
- __skb_push(skb, sizeof(*ph));
- skb_reset_transport_header(skb);
ph = pnp_hdr(skb);
- ph->utid = msg_id; /* whatever */
- ph->message_id = msg_id;
+ ph->utid = 0;
+ ph->message_id = id;
ph->pipe_handle = pn->pipe_handle;
- ph->error_code = PN_PIPE_NO_ERROR;
-
+ ph->data[0] = code;
return pn_skb_send(sk, skb, NULL);
}
-static int pipe_handler_send_created_ind(struct sock *sk, u8 msg_id)
+#define PAD 0x00
+
+#ifdef CONFIG_PHONET_PIPECTRLR
+static int pipe_handler_request(struct sock *sk, u8 id, u8 code,
+ const void *data, int len)
{
- int err_code;
+ struct pep_sock *pn = pep_sk(sk);
struct pnpipehdr *ph;
struct sk_buff *skb;
- struct pep_sock *pn = pep_sk(sk);
- static u8 data[4] = {
- 0x03, 0x04,
- };
- data[2] = pn->tx_fc;
- data[3] = pn->rx_fc;
-
- /*
- * actually, below is number of sub-blocks and not error code.
- * Pipe_created_ind message format does not have any
- * error code field. However, the Phonet stack will always send
- * an error code as part of pnpipehdr. So, use that err_code to
- * specify the number of sub-blocks.
- */
- err_code = 0x01;
-
- skb = alloc_skb(MAX_PNPIPE_HEADER + sizeof(data), GFP_ATOMIC);
+ skb = pep_alloc_skb(sk, data, len, GFP_KERNEL);
if (!skb)
return -ENOMEM;
- skb_set_owner_w(skb, sk);
- skb_reserve(skb, MAX_PNPIPE_HEADER);
- __skb_put(skb, sizeof(data));
- skb_copy_to_linear_data(skb, data, sizeof(data));
- __skb_push(skb, sizeof(*ph));
- skb_reset_transport_header(skb);
ph = pnp_hdr(skb);
- ph->utid = 0;
- ph->message_id = msg_id;
+ ph->utid = id; /* whatever */
+ ph->message_id = id;
ph->pipe_handle = pn->pipe_handle;
- ph->error_code = err_code;
-
+ ph->data[0] = code;
return pn_skb_send(sk, skb, NULL);
}
-static int pipe_handler_send_ind(struct sock *sk, u8 msg_id)
+static int pipe_handler_send_created_ind(struct sock *sk)
{
- int err_code;
- struct pnpipehdr *ph;
- struct sk_buff *skb;
struct pep_sock *pn = pep_sk(sk);
+ u8 data[4] = {
+ PN_PIPE_SB_NEGOTIATED_FC, pep_sb_size(2),
+ pn->tx_fc, pn->rx_fc,
+ };
- /*
- * actually, below is a filler.
- * Pipe_enabled/disabled_ind message format does not have any
- * error code field. However, the Phonet stack will always send
- * an error code as part of pnpipehdr. So, use that err_code to
- * specify the filler value.
- */
- err_code = 0x0;
-
- skb = alloc_skb(MAX_PNPIPE_HEADER, GFP_ATOMIC);
- if (!skb)
- return -ENOMEM;
- skb_set_owner_w(skb, sk);
-
- skb_reserve(skb, MAX_PNPIPE_HEADER);
- __skb_push(skb, sizeof(*ph));
- skb_reset_transport_header(skb);
- ph = pnp_hdr(skb);
- ph->utid = 0;
- ph->message_id = msg_id;
- ph->pipe_handle = pn->pipe_handle;
- ph->error_code = err_code;
+ return pep_indicate(sk, PNS_PIPE_CREATED_IND, 1 /* sub-blocks */,
+ data, 4, GFP_ATOMIC);
+}
- return pn_skb_send(sk, skb, NULL);
+static int pipe_handler_send_ind(struct sock *sk, u8 id)
+{
+ return pep_indicate(sk, id, PAD, NULL, 0, GFP_ATOMIC);
}
static int pipe_handler_enable_pipe(struct sock *sk, int enable)
{
u8 id = enable ? PNS_PEP_ENABLE_REQ : PNS_PEP_DISABLE_REQ;
- return pipe_handler_send_req(sk, id, GFP_KERNEL);
+ return pipe_handler_request(sk, id, PAD, NULL, 0);
}
#endif
@@ -274,23 +218,21 @@ static int pep_ctrlreq_error(struct sock *sk, struct sk_buff *oskb, u8 code,
struct sk_buff *skb;
struct pnpipehdr *ph;
struct sockaddr_pn dst;
+ u8 data[4] = {
+ oph->data[0], /* PEP type */
+ code, /* error code, at an unusual offset */
+ PAD, PAD,
+ };
- skb = alloc_skb(MAX_PNPIPE_HEADER + 4, priority);
+ skb = pep_alloc_skb(sk, data, 4, priority);
if (!skb)
return -ENOMEM;
- skb_set_owner_w(skb, sk);
-
- skb_reserve(skb, MAX_PHONET_HEADER);
- ph = (struct pnpipehdr *)skb_put(skb, sizeof(*ph) + 4);
+ ph = pnp_hdr(skb);
ph->utid = oph->utid;
ph->message_id = PNS_PEP_CTRL_RESP;
ph->pipe_handle = oph->pipe_handle;
ph->data[0] = oph->data[1]; /* CTRL id */
- ph->data[1] = oph->data[0]; /* PEP type */
- ph->data[2] = code; /* error code, at an usual offset */
- ph->data[3] = PAD;
- ph->data[4] = PAD;
pn_skb_get_src_sockaddr(oskb, &dst);
return pn_skb_send(sk, skb, &dst);
@@ -298,34 +240,15 @@ static int pep_ctrlreq_error(struct sock *sk, struct sk_buff *oskb, u8 code,
static int pipe_snd_status(struct sock *sk, u8 type, u8 status, gfp_t priority)
{
- struct pep_sock *pn = pep_sk(sk);
- struct pnpipehdr *ph;
- struct sk_buff *skb;
+ u8 data[4] = { type, PAD, PAD, status };
- skb = alloc_skb(MAX_PNPIPE_HEADER + 4, priority);
- if (!skb)
- return -ENOMEM;
- skb_set_owner_w(skb, sk);
-
- skb_reserve(skb, MAX_PNPIPE_HEADER + 4);
- __skb_push(skb, sizeof(*ph) + 4);
- skb_reset_transport_header(skb);
- ph = pnp_hdr(skb);
- ph->utid = 0;
- ph->message_id = PNS_PEP_STATUS_IND;
- ph->pipe_handle = pn->pipe_handle;
- ph->pep_type = PN_PEP_TYPE_COMMON;
- ph->data[1] = type;
- ph->data[2] = PAD;
- ph->data[3] = PAD;
- ph->data[4] = status;
-
- return pn_skb_send(sk, skb, NULL);
+ return pep_indicate(sk, PNS_PEP_STATUS_IND, PN_PEP_TYPE_COMMON,
+ data, 4, priority);
}
/* Send our RX flow control information to the sender.
* Socket must be locked. */
-static void pipe_grant_credits(struct sock *sk)
+static void pipe_grant_credits(struct sock *sk, gfp_t priority)
{
struct pep_sock *pn = pep_sk(sk);
@@ -335,16 +258,16 @@ static void pipe_grant_credits(struct sock *sk)
case PN_LEGACY_FLOW_CONTROL: /* TODO */
break;
case PN_ONE_CREDIT_FLOW_CONTROL:
- pipe_snd_status(sk, PN_PEP_IND_FLOW_CONTROL,
- PEP_IND_READY, GFP_ATOMIC);
- pn->rx_credits = 1;
+ if (pipe_snd_status(sk, PN_PEP_IND_FLOW_CONTROL,
+ PEP_IND_READY, priority) == 0)
+ pn->rx_credits = 1;
break;
case PN_MULTI_CREDIT_FLOW_CONTROL:
if ((pn->rx_credits + CREDITS_THR) > CREDITS_MAX)
break;
if (pipe_snd_status(sk, PN_PEP_IND_ID_MCFC_GRANT_CREDITS,
CREDITS_MAX - pn->rx_credits,
- GFP_ATOMIC) == 0)
+ priority) == 0)
pn->rx_credits = CREDITS_MAX;
break;
}
@@ -474,7 +397,7 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
if (sk->sk_state == TCP_ESTABLISHED)
break; /* Nothing to do */
sk->sk_state = TCP_ESTABLISHED;
- pipe_grant_credits(sk);
+ pipe_grant_credits(sk, GFP_ATOMIC);
break;
#endif
@@ -561,7 +484,7 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
if (sk->sk_state == TCP_ESTABLISHED)
break; /* Nothing to do */
sk->sk_state = TCP_ESTABLISHED;
- pipe_grant_credits(sk);
+ pipe_grant_credits(sk, GFP_ATOMIC);
break;
case PNS_PIPE_DISABLED_IND:
@@ -655,7 +578,7 @@ static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
pn->rx_credits = 0;
sk->sk_state_change(sk);
- return pipe_handler_send_created_ind(sk, PNS_PIPE_CREATED_IND);
+ return pipe_handler_send_created_ind(sk);
}
#endif
@@ -853,19 +776,15 @@ static int pipe_do_remove(struct sock *sk)
struct pnpipehdr *ph;
struct sk_buff *skb;
- skb = alloc_skb(MAX_PNPIPE_HEADER, GFP_KERNEL);
+ skb = pep_alloc_skb(sk, NULL, 0, GFP_KERNEL);
if (!skb)
return -ENOMEM;
- skb_reserve(skb, MAX_PNPIPE_HEADER);
- __skb_push(skb, sizeof(*ph));
- skb_reset_transport_header(skb);
ph = pnp_hdr(skb);
ph->utid = 0;
ph->message_id = PNS_PIPE_REMOVE_REQ;
ph->pipe_handle = pn->pipe_handle;
ph->data[0] = PAD;
-
return pn_skb_send(sk, skb, NULL);
}
#endif
@@ -894,7 +813,7 @@ static void pep_sock_close(struct sock *sk, long timeout)
pipe_do_remove(sk);
#else
/* send pep disconnect request */
- pipe_handler_send_req(sk, PNS_PEP_DISCONNECT_REQ, GFP_KERNEL);
+ pipe_handler_request(sk, PNS_PEP_DISCONNECT_REQ, PAD, NULL, 0);
sk->sk_state = TCP_CLOSE;
#endif
}
@@ -980,10 +899,12 @@ static int pep_sock_connect(struct sock *sk, struct sockaddr *addr, int len)
{
struct pep_sock *pn = pep_sk(sk);
const struct sockaddr_pn *spn = (struct sockaddr_pn *)addr;
+ u8 data[4] = { 0 /* sub-blocks */, PAD, PAD, PAD };
pn->pn_sk.dobject = pn_sockaddr_get_object(spn);
pn->pn_sk.resource = pn_sockaddr_get_resource(spn);
- return pipe_handler_send_req(sk, PNS_PEP_CONNECT_REQ, GFP_KERNEL);
+ return pipe_handler_request(sk, PNS_PEP_CONNECT_REQ,
+ PN_PIPE_DISABLE, data, 4);
}
#endif
@@ -1280,7 +1201,7 @@ struct sk_buff *pep_read(struct sock *sk)
struct sk_buff *skb = skb_dequeue(&sk->sk_receive_queue);
if (sk->sk_state == TCP_ESTABLISHED)
- pipe_grant_credits(sk);
+ pipe_grant_credits(sk, GFP_ATOMIC);
return skb;
}
@@ -1325,7 +1246,7 @@ static int pep_recvmsg(struct kiocb *iocb, struct sock *sk,
}
if (sk->sk_state == TCP_ESTABLISHED)
- pipe_grant_credits(sk);
+ pipe_grant_credits(sk, GFP_KERNEL);
release_sock(sk);
copy:
msg->msg_flags |= MSG_EOR;
--
1.7.1
^ permalink raw reply related
* [PATCH 3/8] Phonet: correct pipe backlog callback return values
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
In some cases, the Phonet pipe backlog callbacks returned negative
errno instead of NET_RX_* values.
In other cases, NET_RX_DROP was returned for invalid packets, even
though it seems only intended for buffering problems (not for
deliberately discarded packets).
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
net/phonet/pep.c | 25 +++++++++++--------------
1 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 875e86c..40952c7 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -522,7 +522,8 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
if (!pn_flow_safe(pn->rx_fc)) {
err = sock_queue_rcv_skb(sk, skb);
if (!err)
- return 0;
+ return NET_RX_SUCCESS;
+ err = -ENOBUFS;
break;
}
@@ -575,7 +576,7 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
}
out:
kfree_skb(skb);
- return err;
+ return (err == -ENOBUFS) ? NET_RX_DROP : NET_RX_SUCCESS;
queue:
skb->dev = NULL;
@@ -584,7 +585,7 @@ queue:
skb_queue_tail(queue, skb);
if (!sock_flag(sk, SOCK_DEAD))
sk->sk_data_ready(sk, err);
- return 0;
+ return NET_RX_SUCCESS;
}
/* Destroy connected sock. */
@@ -686,11 +687,6 @@ static int pep_connreq_rcv(struct sock *sk, struct sk_buff *skb)
}
peer_type = hdr->other_pep_type << 8;
- if (unlikely(sk->sk_state != TCP_LISTEN) || sk_acceptq_is_full(sk)) {
- pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE);
- return -ENOBUFS;
- }
-
/* Parse sub-blocks (options) */
n_sb = hdr->data[4];
while (n_sb > 0) {
@@ -790,7 +786,6 @@ static int pep_do_rcv(struct sock *sk, struct sk_buff *skb)
struct sock *sknode;
struct pnpipehdr *hdr;
struct sockaddr_pn dst;
- int err = NET_RX_SUCCESS;
u8 pipe_handle;
if (!pskb_may_pull(skb, sizeof(*hdr)))
@@ -814,18 +809,20 @@ static int pep_do_rcv(struct sock *sk, struct sk_buff *skb)
sock_put(sknode);
if (net_ratelimit())
printk(KERN_WARNING"Phonet unconnected PEP ignored");
- err = NET_RX_DROP;
goto drop;
}
switch (hdr->message_id) {
case PNS_PEP_CONNECT_REQ:
- err = pep_connreq_rcv(sk, skb);
+ if (sk->sk_state == TCP_LISTEN && !sk_acceptq_is_full(sk))
+ pep_connreq_rcv(sk, skb);
+ else
+ pep_reject_conn(sk, skb, PN_PIPE_ERR_PEP_IN_USE);
break;
#ifdef CONFIG_PHONET_PIPECTRLR
case PNS_PEP_CONNECT_RESP:
- err = pep_connresp_rcv(sk, skb);
+ pep_connresp_rcv(sk, skb);
break;
#endif
@@ -842,11 +839,11 @@ static int pep_do_rcv(struct sock *sk, struct sk_buff *skb)
case PNS_PEP_DISABLE_REQ:
/* invalid handle is not even allowed here! */
default:
- err = NET_RX_DROP;
+ break;
}
drop:
kfree_skb(skb);
- return err;
+ return NET_RX_SUCCESS;
}
#ifndef CONFIG_PHONET_PIPECTRLR
--
1.7.1
^ permalink raw reply related
* [PATCH 2/8] Phonet: return an error when packet TX fails
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
Phonet assumes that packets are never dropped. We try our best to
avoid this situation. But lets return ENOBUFS if queueing to the
network device fails so that the caller knows things went wrong.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
net/phonet/af_phonet.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
index 4706b77..c6fffd9 100644
--- a/net/phonet/af_phonet.c
+++ b/net/phonet/af_phonet.c
@@ -195,11 +195,7 @@ static int pn_send(struct sk_buff *skb, struct net_device *dev,
if (skb->pkt_type == PACKET_LOOPBACK) {
skb_reset_mac_header(skb);
skb_orphan(skb);
- if (irq)
- netif_rx(skb);
- else
- netif_rx_ni(skb);
- err = 0;
+ err = (irq ? netif_rx(skb) : netif_rx_ni(skb)) ? -ENOBUFS : 0;
} else {
err = dev_hard_header(skb, dev, ntohs(skb->protocol),
NULL, NULL, skb->len);
@@ -208,6 +204,8 @@ static int pn_send(struct sk_buff *skb, struct net_device *dev,
goto drop;
}
err = dev_queue_xmit(skb);
+ if (unlikely(err > 0))
+ err = net_xmit_errno(err);
}
return err;
--
1.7.1
^ permalink raw reply related
* [PATCH 1/8] Phonet: fix NULL dereference on TX path with implicit source
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
The previous Phonet patch series introduced per-socket implicit
destination (i.e. connect()). In that case, the destination
socket address is NULL in the transmit function.
However commit a8059512b120362b15424f152b2548fe8b11bd0c
("Phonet: implement per-socket destination/peer address")
is incomplete and would trigger a NULL dereference.
(Fortunately, the code is not in released kernel, and in fact
currently not reachable.)
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
net/phonet/af_phonet.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
index 30cc676..4706b77 100644
--- a/net/phonet/af_phonet.c
+++ b/net/phonet/af_phonet.c
@@ -262,10 +262,9 @@ int pn_skb_send(struct sock *sk, struct sk_buff *skb,
else if (phonet_address_lookup(net, daddr) == 0) {
dev = phonet_device_get(net);
skb->pkt_type = PACKET_LOOPBACK;
- } else if (pn_sockaddr_get_object(target) == 0) {
+ } else if (dst == 0) {
/* Resource routing (small race until phonet_rcv()) */
- struct sock *sk = pn_find_sock_by_res(net,
- target->spn_resource);
+ struct sock *sk = pn_find_sock_by_res(net, res);
if (sk) {
sock_put(sk);
dev = phonet_device_get(net);
--
1.7.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