Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On 08/28/2015 08:25 PM, Vlad Yasevich wrote: > On 08/27/2015 10:42 PM, Jason Wang wrote: >> > >> > >> > On 08/27/2015 06:43 PM, Michael S. Tsirkin wrote: >>> >> On Wed, Aug 26, 2015 at 01:45:30PM +0800, Jason Wang wrote: >>> >>> On 08/26/2015 12:32 AM, Vlad Yasevich wrote: > On 08/25/2015 07:30 AM, Jason Wang wrote: >> > On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: >>> >> On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: > For macvlan, switch to use IFF_NO_QUEUE instead of > tx_queue_len = 0. > > For macvtap, after commit > 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 > ("macvtap: Add support of packet capture on macvtap > device."). Multiqueue macvtap suffers from single qdisc lock > contention. This is because macvtap claims a non zero > tx_queue_len and > it reuses this value as it socket receive queue size.Thanks > to > IFF_NO_QUEUE, we can remove the lock contention without > breaking > existing socket receive queue length logic. > > Cc: Patrick McHardy > Cc: Vladislav Yasevich > Cc: Michael S. Tsirkin > Signed-off-by: Jason Wang >>> >> Seems to make sense. Give me a day or two to get over the jet lag >>> >> (and get out from under the pile of mail accumulated while I was >>> >> traveling), >>> >> I'll review properly and ack. >>> >> >> > A note on this patch: only default qdisc were removed but we don't >> > lose >> > the ability to attach a qdisc to macvtap (though it may cause lock >> > contention on multiqueue case). >> > > Wouldn't that lock contention be solved if we really had multiple > queues > for multi-queue macvtaps? > > -vlad >>> Yes, but this introduce another layer of txq locks contention? >>> >> I don't follow - why does it? Could you clarify please? >> > >> > I believe Vlad wants to remove NETIF_F_LLTX. If yes, core will do an >> > extra tx lock at macvlan layer. > No, I don't want to remove it. In a sense, it would function similar to > how it works when fwd_priv is populated. I am still testing the code > as it's showing some strange artifacts... could be due to keeping LLTX. > > -vlad > I see. I'm ok to wait for your code. But if a patch of just two lines works, probably no need to try complex method. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On 08/27/2015 10:42 PM, Jason Wang wrote: > > > On 08/27/2015 06:43 PM, Michael S. Tsirkin wrote: >> On Wed, Aug 26, 2015 at 01:45:30PM +0800, Jason Wang wrote: >>> >>> On 08/26/2015 12:32 AM, Vlad Yasevich wrote: On 08/25/2015 07:30 AM, Jason Wang wrote: > On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: >> On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 ("macvtap: Add support of packet capture on macvtap device."). Multiqueue macvtap suffers from single qdisc lock contention. This is because macvtap claims a non zero tx_queue_len and it reuses this value as it socket receive queue size.Thanks to IFF_NO_QUEUE, we can remove the lock contention without breaking existing socket receive queue length logic. Cc: Patrick McHardy Cc: Vladislav Yasevich Cc: Michael S. Tsirkin Signed-off-by: Jason Wang >> Seems to make sense. Give me a day or two to get over the jet lag >> (and get out from under the pile of mail accumulated while I was >> traveling), >> I'll review properly and ack. >> > A note on this patch: only default qdisc were removed but we don't lose > the ability to attach a qdisc to macvtap (though it may cause lock > contention on multiqueue case). > Wouldn't that lock contention be solved if we really had multiple queues for multi-queue macvtaps? -vlad >>> Yes, but this introduce another layer of txq locks contention? >> I don't follow - why does it? Could you clarify please? > > I believe Vlad wants to remove NETIF_F_LLTX. If yes, core will do an > extra tx lock at macvlan layer. No, I don't want to remove it. In a sense, it would function similar to how it works when fwd_priv is populated. I am still testing the code as it's showing some strange artifacts... could be due to keeping LLTX. -vlad > >> >>> And it >>> also needs macvlan multiqueue support. We used to do something like this >>> but switch to NETIF_F_LLTX finally. You may refer: >>> >>> 2c11455321f37da6fe6cc36353149f9ac9183334 macvlan: add multiqueue capability >>> 8ffab51b3dfc54876f145f15b351c41f3f703195 macvlan: lockless tx path >> My concern is that the moment someone configures a non-standard qdisc >> scalability suddenly disappears. That would also be tricky to debug in the >> field as not a lot of developers use non-standard qdiscs. >> What do you think? > > Probably not an issue. Non-standard qdisc may need be attached manually > after device creation, and we don't lose this ability with this patch. > (Unless somebody changes default_qdisc). Actually, user before > 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 does not expect any qdisc work > for macvtap like other stacked devices. This patch also restore this. > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On 08/27/2015 06:43 PM, Michael S. Tsirkin wrote: > On Wed, Aug 26, 2015 at 01:45:30PM +0800, Jason Wang wrote: >> >> On 08/26/2015 12:32 AM, Vlad Yasevich wrote: >>> On 08/25/2015 07:30 AM, Jason Wang wrote: On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: > On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: >>> For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. >>> >>> For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 >>> ("macvtap: Add support of packet capture on macvtap >>> device."). Multiqueue macvtap suffers from single qdisc lock >>> contention. This is because macvtap claims a non zero tx_queue_len and >>> it reuses this value as it socket receive queue size.Thanks to >>> IFF_NO_QUEUE, we can remove the lock contention without breaking >>> existing socket receive queue length logic. >>> >>> Cc: Patrick McHardy >>> Cc: Vladislav Yasevich >>> Cc: Michael S. Tsirkin >>> Signed-off-by: Jason Wang > Seems to make sense. Give me a day or two to get over the jet lag > (and get out from under the pile of mail accumulated while I was > traveling), > I'll review properly and ack. > A note on this patch: only default qdisc were removed but we don't lose the ability to attach a qdisc to macvtap (though it may cause lock contention on multiqueue case). >>> Wouldn't that lock contention be solved if we really had multiple queues >>> for multi-queue macvtaps? >>> >>> -vlad >> Yes, but this introduce another layer of txq locks contention? > I don't follow - why does it? Could you clarify please? I believe Vlad wants to remove NETIF_F_LLTX. If yes, core will do an extra tx lock at macvlan layer. > >> And it >> also needs macvlan multiqueue support. We used to do something like this >> but switch to NETIF_F_LLTX finally. You may refer: >> >> 2c11455321f37da6fe6cc36353149f9ac9183334 macvlan: add multiqueue capability >> 8ffab51b3dfc54876f145f15b351c41f3f703195 macvlan: lockless tx path > My concern is that the moment someone configures a non-standard qdisc > scalability suddenly disappears. That would also be tricky to debug in the > field as not a lot of developers use non-standard qdiscs. > What do you think? Probably not an issue. Non-standard qdisc may need be attached manually after device creation, and we don't lose this ability with this patch. (Unless somebody changes default_qdisc). Actually, user before 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 does not expect any qdisc work for macvtap like other stacked devices. This patch also restore this. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On Wed, Aug 26, 2015 at 01:45:30PM +0800, Jason Wang wrote: > > > On 08/26/2015 12:32 AM, Vlad Yasevich wrote: > > On 08/25/2015 07:30 AM, Jason Wang wrote: > >> > >> On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: > >>> On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: > > For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. > > > > For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 > > ("macvtap: Add support of packet capture on macvtap > > device."). Multiqueue macvtap suffers from single qdisc lock > > contention. This is because macvtap claims a non zero tx_queue_len and > > it reuses this value as it socket receive queue size.Thanks to > > IFF_NO_QUEUE, we can remove the lock contention without breaking > > existing socket receive queue length logic. > > > > Cc: Patrick McHardy > > Cc: Vladislav Yasevich > > Cc: Michael S. Tsirkin > > Signed-off-by: Jason Wang > >>> Seems to make sense. Give me a day or two to get over the jet lag > >>> (and get out from under the pile of mail accumulated while I was > >>> traveling), > >>> I'll review properly and ack. > >>> > >> A note on this patch: only default qdisc were removed but we don't lose > >> the ability to attach a qdisc to macvtap (though it may cause lock > >> contention on multiqueue case). > >> > > Wouldn't that lock contention be solved if we really had multiple queues > > for multi-queue macvtaps? > > > > -vlad > > Yes, but this introduce another layer of txq locks contention? I don't follow - why does it? Could you clarify please? > And it > also needs macvlan multiqueue support. We used to do something like this > but switch to NETIF_F_LLTX finally. You may refer: > > 2c11455321f37da6fe6cc36353149f9ac9183334 macvlan: add multiqueue capability > 8ffab51b3dfc54876f145f15b351c41f3f703195 macvlan: lockless tx path My concern is that the moment someone configures a non-standard qdisc scalability suddenly disappears. That would also be tricky to debug in the field as not a lot of developers use non-standard qdiscs. What do you think? -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On 08/26/2015 12:32 AM, Vlad Yasevich wrote: > On 08/25/2015 07:30 AM, Jason Wang wrote: >> >> On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: >>> On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: > For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. > > For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 > ("macvtap: Add support of packet capture on macvtap > device."). Multiqueue macvtap suffers from single qdisc lock > contention. This is because macvtap claims a non zero tx_queue_len and > it reuses this value as it socket receive queue size.Thanks to > IFF_NO_QUEUE, we can remove the lock contention without breaking > existing socket receive queue length logic. > > Cc: Patrick McHardy > Cc: Vladislav Yasevich > Cc: Michael S. Tsirkin > Signed-off-by: Jason Wang >>> Seems to make sense. Give me a day or two to get over the jet lag >>> (and get out from under the pile of mail accumulated while I was traveling), >>> I'll review properly and ack. >>> >> A note on this patch: only default qdisc were removed but we don't lose >> the ability to attach a qdisc to macvtap (though it may cause lock >> contention on multiqueue case). >> > Wouldn't that lock contention be solved if we really had multiple queues > for multi-queue macvtaps? > > -vlad Yes, but this introduce another layer of txq locks contention? And it also needs macvlan multiqueue support. We used to do something like this but switch to NETIF_F_LLTX finally. You may refer: 2c11455321f37da6fe6cc36353149f9ac9183334 macvlan: add multiqueue capability 8ffab51b3dfc54876f145f15b351c41f3f703195 macvlan: lockless tx path -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On 08/25/2015 07:30 AM, Jason Wang wrote: > > > On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: >> On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 ("macvtap: Add support of packet capture on macvtap device."). Multiqueue macvtap suffers from single qdisc lock contention. This is because macvtap claims a non zero tx_queue_len and it reuses this value as it socket receive queue size.Thanks to IFF_NO_QUEUE, we can remove the lock contention without breaking existing socket receive queue length logic. Cc: Patrick McHardy Cc: Vladislav Yasevich Cc: Michael S. Tsirkin Signed-off-by: Jason Wang >> Seems to make sense. Give me a day or two to get over the jet lag >> (and get out from under the pile of mail accumulated while I was traveling), >> I'll review properly and ack. >> > > A note on this patch: only default qdisc were removed but we don't lose > the ability to attach a qdisc to macvtap (though it may cause lock > contention on multiqueue case). > Wouldn't that lock contention be solved if we really had multiple queues for multi-queue macvtaps? -vlad -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On 08/25/2015 06:17 PM, Michael S. Tsirkin wrote: > On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: >> > For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. >> > >> > For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 >> > ("macvtap: Add support of packet capture on macvtap >> > device."). Multiqueue macvtap suffers from single qdisc lock >> > contention. This is because macvtap claims a non zero tx_queue_len and >> > it reuses this value as it socket receive queue size.Thanks to >> > IFF_NO_QUEUE, we can remove the lock contention without breaking >> > existing socket receive queue length logic. >> > >> > Cc: Patrick McHardy >> > Cc: Vladislav Yasevich >> > Cc: Michael S. Tsirkin >> > Signed-off-by: Jason Wang > Seems to make sense. Give me a day or two to get over the jet lag > (and get out from under the pile of mail accumulated while I was traveling), > I'll review properly and ack. > A note on this patch: only default qdisc were removed but we don't lose the ability to attach a qdisc to macvtap (though it may cause lock contention on multiqueue case). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] macvtap/macvlan: use IFF_NO_QUEUE
On Mon, Aug 24, 2015 at 04:33:12PM +0800, Jason Wang wrote: > For macvlan, switch to use IFF_NO_QUEUE instead of tx_queue_len = 0. > > For macvtap, after commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523 > ("macvtap: Add support of packet capture on macvtap > device."). Multiqueue macvtap suffers from single qdisc lock > contention. This is because macvtap claims a non zero tx_queue_len and > it reuses this value as it socket receive queue size.Thanks to > IFF_NO_QUEUE, we can remove the lock contention without breaking > existing socket receive queue length logic. > > Cc: Patrick McHardy > Cc: Vladislav Yasevich > Cc: Michael S. Tsirkin > Signed-off-by: Jason Wang Seems to make sense. Give me a day or two to get over the jet lag (and get out from under the pile of mail accumulated while I was traveling), I'll review properly and ack. > --- > drivers/net/macvlan.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > index 47da435..09d8718 100644 > --- a/drivers/net/macvlan.c > +++ b/drivers/net/macvlan.c > @@ -1056,7 +1056,7 @@ void macvlan_common_setup(struct net_device *dev) > > dev->priv_flags&= ~IFF_TX_SKB_SHARING; > netif_keep_dst(dev); > - dev->priv_flags|= IFF_UNICAST_FLT; > + dev->priv_flags|= IFF_UNICAST_FLT | IFF_NO_QUEUE; > dev->netdev_ops = &macvlan_netdev_ops; > dev->destructor = free_netdev; > dev->header_ops = &macvlan_hard_header_ops; > @@ -1067,7 +1067,6 @@ EXPORT_SYMBOL_GPL(macvlan_common_setup); > static void macvlan_setup(struct net_device *dev) > { > macvlan_common_setup(dev); > - dev->tx_queue_len = 0; > } > > static int macvlan_port_create(struct net_device *dev) > -- > 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/