* Re: [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-21 12:57 UTC (permalink / raw)
To: Liran Alon
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <BB2B938F-D2F0-47F8-B651-70EC8A19BC6B@oracle.com>
On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
>
>
> > On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
> >>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
> >>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
> >>>>>>
> >>>>>> The set of changes I vision to fix our issues are:
> >>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
> >>>>>> (E.g. Configure the net-failover VF slave in some special way).
> >>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
> >>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
> >>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
> >>>>>>
> >>>>>> Is there any clear issue we see regarding the above suggestion?
> >>>>>>
> >>>>>> -Liran
> >>>>>
> >>>>> The issue would be this: how do we avoid conflicting with namespaces
> >>>>> created by users?
> >>>>
> >>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
> >>>> To reference a hidden netns, you need to do it explicitly.
> >>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
> >>>
> >>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
> >>
> >> This is also a good idea that will solve the issue. Yes.
> >>
> >>>
> >>>> Does this seems reasonable?
> >>>>
> >>>> -Liran
> >>>
> >>> Reasonable I'd say yes, easy to implement probably no. But maybe I
> >>> missed a trick or two.
> >>
> >> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
> >> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
> >> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
> >> the breakage we have today for the various userspace components.
> >>
> >> -Liran
> >
> > It seems quite easy to supply that as a module parameter. Do we need two
> > namespaces though? Won't some userspace still be confused by the two
> > slaves sharing the MAC address?
>
> That’s one reasonable option.
> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
>
> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
>
> -Liran
OK. Now what happens if master is moved to another namespace? Do we need
to move the slaves too?
Also siwei's patch is then kind of extraneous right?
Attempts to rename a slave will now fail as it's in a namespace...
> >
> > --
> > MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Liran Alon @ 2019-03-21 13:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321085159-mutt-send-email-mst@kernel.org>
> On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
>>
>>
>>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
>>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
>>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
>>>>>>>>
>>>>>>>> The set of changes I vision to fix our issues are:
>>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
>>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
>>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
>>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
>>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
>>>>>>>>
>>>>>>>> Is there any clear issue we see regarding the above suggestion?
>>>>>>>>
>>>>>>>> -Liran
>>>>>>>
>>>>>>> The issue would be this: how do we avoid conflicting with namespaces
>>>>>>> created by users?
>>>>>>
>>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
>>>>>> To reference a hidden netns, you need to do it explicitly.
>>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
>>>>>
>>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
>>>>
>>>> This is also a good idea that will solve the issue. Yes.
>>>>
>>>>>
>>>>>> Does this seems reasonable?
>>>>>>
>>>>>> -Liran
>>>>>
>>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
>>>>> missed a trick or two.
>>>>
>>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
>>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
>>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
>>>> the breakage we have today for the various userspace components.
>>>>
>>>> -Liran
>>>
>>> It seems quite easy to supply that as a module parameter. Do we need two
>>> namespaces though? Won't some userspace still be confused by the two
>>> slaves sharing the MAC address?
>>
>> That’s one reasonable option.
>> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
>> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
>>
>> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
>>
>> -Liran
>
> OK. Now what happens if master is moved to another namespace? Do we need
> to move the slaves too?
No. Why would we move the slaves? The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>
> Also siwei's patch is then kind of extraneous right?
> Attempts to rename a slave will now fail as it's in a namespace…
I’m not sure actually. Isn't udev/systemd netns-aware?
I would expect it to be able to provide names also to netdevs in netns different than default netns.
If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
-Liran
>
>>>
>>> --
>>> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-21 13:12 UTC (permalink / raw)
To: Liran Alon
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <2939FB15-720A-4C9E-92B7-2DBA139DDE0F@oracle.com>
On Thu, Mar 21, 2019 at 03:04:37PM +0200, Liran Alon wrote:
>
>
> > On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
> >>
> >>
> >>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>
> >>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
> >>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
> >>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
> >>>>>>>>
> >>>>>>>> The set of changes I vision to fix our issues are:
> >>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
> >>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
> >>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
> >>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
> >>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
> >>>>>>>>
> >>>>>>>> Is there any clear issue we see regarding the above suggestion?
> >>>>>>>>
> >>>>>>>> -Liran
> >>>>>>>
> >>>>>>> The issue would be this: how do we avoid conflicting with namespaces
> >>>>>>> created by users?
> >>>>>>
> >>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
> >>>>>> To reference a hidden netns, you need to do it explicitly.
> >>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
> >>>>>
> >>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
> >>>>
> >>>> This is also a good idea that will solve the issue. Yes.
> >>>>
> >>>>>
> >>>>>> Does this seems reasonable?
> >>>>>>
> >>>>>> -Liran
> >>>>>
> >>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
> >>>>> missed a trick or two.
> >>>>
> >>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
> >>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
> >>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
> >>>> the breakage we have today for the various userspace components.
> >>>>
> >>>> -Liran
> >>>
> >>> It seems quite easy to supply that as a module parameter. Do we need two
> >>> namespaces though? Won't some userspace still be confused by the two
> >>> slaves sharing the MAC address?
> >>
> >> That’s one reasonable option.
> >> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
> >> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
> >>
> >> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
> >>
> >> -Liran
> >
> > OK. Now what happens if master is moved to another namespace? Do we need
> > to move the slaves too?
>
> No. Why would we move the slaves?
The reason we have 3 device model at all is so users can fine tune the
slaves. I don't see why this applies to the root namespace but not
a container. If it has access to failover it should have access
to slaves.
> The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
So that makes the common case easy. That is good. My worry is it might
make some uncommon cases impossible.
> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>
> >
> > Also siwei's patch is then kind of extraneous right?
> > Attempts to rename a slave will now fail as it's in a namespace…
>
> I’m not sure actually. Isn't udev/systemd netns-aware?
> I would expect it to be able to provide names also to netdevs in netns different than default netns.
I think most people move devices after they are renamed.
> If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
>
> -Liran
>
> >
> >>>
> >>> --
> >>> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Liran Alon @ 2019-03-21 13:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321090619-mutt-send-email-mst@kernel.org>
> On 21 Mar 2019, at 15:12, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 21, 2019 at 03:04:37PM +0200, Liran Alon wrote:
>>
>>
>>> On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
>>>>
>>>>
>>>>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>
>>>>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
>>>>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
>>>>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
>>>>>>>>>>
>>>>>>>>>> The set of changes I vision to fix our issues are:
>>>>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
>>>>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
>>>>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
>>>>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
>>>>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
>>>>>>>>>>
>>>>>>>>>> Is there any clear issue we see regarding the above suggestion?
>>>>>>>>>>
>>>>>>>>>> -Liran
>>>>>>>>>
>>>>>>>>> The issue would be this: how do we avoid conflicting with namespaces
>>>>>>>>> created by users?
>>>>>>>>
>>>>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
>>>>>>>> To reference a hidden netns, you need to do it explicitly.
>>>>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
>>>>>>>
>>>>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
>>>>>>
>>>>>> This is also a good idea that will solve the issue. Yes.
>>>>>>
>>>>>>>
>>>>>>>> Does this seems reasonable?
>>>>>>>>
>>>>>>>> -Liran
>>>>>>>
>>>>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
>>>>>>> missed a trick or two.
>>>>>>
>>>>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
>>>>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
>>>>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
>>>>>> the breakage we have today for the various userspace components.
>>>>>>
>>>>>> -Liran
>>>>>
>>>>> It seems quite easy to supply that as a module parameter. Do we need two
>>>>> namespaces though? Won't some userspace still be confused by the two
>>>>> slaves sharing the MAC address?
>>>>
>>>> That’s one reasonable option.
>>>> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
>>>> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
>>>>
>>>> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
>>>>
>>>> -Liran
>>>
>>> OK. Now what happens if master is moved to another namespace? Do we need
>>> to move the slaves too?
>>
>> No. Why would we move the slaves?
>
>
> The reason we have 3 device model at all is so users can fine tune the
> slaves.
I Agree.
> I don't see why this applies to the root namespace but not
> a container. If it has access to failover it should have access
> to slaves.
Oh now I see your point. I haven’t thought about the containers usage.
My thinking was that customer can always just enter to the “hidden” netns and configure there whatever he wants.
Do you have a suggestion how to handle this?
One option can be that every "visible" netns on system will have a “hidden” unnamed netns where the net-failover slaves reside in.
If customer wishes to be able to enter to that netns and manage the net-failover slaves explicitly, it will need to have an updated iproute2
that knows how to enter to that hidden netns. For most customers, they won’t need to ever enter that netns and thus it is ok they don’t
have this updated iproute2.
>
>> The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
>
> So that makes the common case easy. That is good. My worry is it might
> make some uncommon cases impossible.
>
>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>>
>>>
>>> Also siwei's patch is then kind of extraneous right?
>>> Attempts to rename a slave will now fail as it's in a namespace…
>>
>> I’m not sure actually. Isn't udev/systemd netns-aware?
>> I would expect it to be able to provide names also to netdevs in netns different than default netns.
>
> I think most people move devices after they are renamed.
So?
Si-Wei patch handles the issue that resolves from the fact the net-failover master will be opened before the rename on the net-failover slaves occur.
This should happen (to my understanding) regardless of network namespaces.
-Liran
>
>> If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
>>
>> -Liran
>>
>>>
>>>>>
>>>>> --
>>>>> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-21 13:51 UTC (permalink / raw)
To: Liran Alon
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <1B52153B-B968-4E5B-8959-E7E83CE7FEAF@oracle.com>
On Thu, Mar 21, 2019 at 03:24:39PM +0200, Liran Alon wrote:
>
>
> > On 21 Mar 2019, at 15:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 03:04:37PM +0200, Liran Alon wrote:
> >>
> >>
> >>> On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>
> >>> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
> >>>>
> >>>>
> >>>>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>
> >>>>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
> >>>>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
> >>>>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
> >>>>>>>>>>
> >>>>>>>>>> The set of changes I vision to fix our issues are:
> >>>>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
> >>>>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
> >>>>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
> >>>>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
> >>>>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
> >>>>>>>>>>
> >>>>>>>>>> Is there any clear issue we see regarding the above suggestion?
> >>>>>>>>>>
> >>>>>>>>>> -Liran
> >>>>>>>>>
> >>>>>>>>> The issue would be this: how do we avoid conflicting with namespaces
> >>>>>>>>> created by users?
> >>>>>>>>
> >>>>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
> >>>>>>>> To reference a hidden netns, you need to do it explicitly.
> >>>>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
> >>>>>>>
> >>>>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
> >>>>>>
> >>>>>> This is also a good idea that will solve the issue. Yes.
> >>>>>>
> >>>>>>>
> >>>>>>>> Does this seems reasonable?
> >>>>>>>>
> >>>>>>>> -Liran
> >>>>>>>
> >>>>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
> >>>>>>> missed a trick or two.
> >>>>>>
> >>>>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
> >>>>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
> >>>>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
> >>>>>> the breakage we have today for the various userspace components.
> >>>>>>
> >>>>>> -Liran
> >>>>>
> >>>>> It seems quite easy to supply that as a module parameter. Do we need two
> >>>>> namespaces though? Won't some userspace still be confused by the two
> >>>>> slaves sharing the MAC address?
> >>>>
> >>>> That’s one reasonable option.
> >>>> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
> >>>> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
> >>>>
> >>>> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
> >>>>
> >>>> -Liran
> >>>
> >>> OK. Now what happens if master is moved to another namespace? Do we need
> >>> to move the slaves too?
> >>
> >> No. Why would we move the slaves?
> >
> >
> > The reason we have 3 device model at all is so users can fine tune the
> > slaves.
>
> I Agree.
>
> > I don't see why this applies to the root namespace but not
> > a container. If it has access to failover it should have access
> > to slaves.
>
> Oh now I see your point. I haven’t thought about the containers usage.
> My thinking was that customer can always just enter to the “hidden” netns and configure there whatever he wants.
>
> Do you have a suggestion how to handle this?
>
> One option can be that every "visible" netns on system will have a “hidden” unnamed netns where the net-failover slaves reside in.
> If customer wishes to be able to enter to that netns and manage the net-failover slaves explicitly, it will need to have an updated iproute2
> that knows how to enter to that hidden netns. For most customers, they won’t need to ever enter that netns and thus it is ok they don’t
> have this updated iproute2.
Right so slaves need to be moved whenever master is moved.
Given the amount of mess involved, should we just teach
userspace to create the hidden netns and move slaves there?
> >
> >> The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
> >
> > So that makes the common case easy. That is good. My worry is it might
> > make some uncommon cases impossible.
> >
> >> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
> >>
> >>>
> >>> Also siwei's patch is then kind of extraneous right?
> >>> Attempts to rename a slave will now fail as it's in a namespace…
> >>
> >> I’m not sure actually. Isn't udev/systemd netns-aware?
> >> I would expect it to be able to provide names also to netdevs in netns different than default netns.
> >
> > I think most people move devices after they are renamed.
>
> So?
> Si-Wei patch handles the issue that resolves from the fact the net-failover master will be opened before the rename on the net-failover slaves occur.
> This should happen (to my understanding) regardless of network namespaces.
>
> -Liran
My point was that any tool that moves devices after they
are renamed will be broken by kernel automatically putting
them in a namespace.
> >
> >> If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
> >>
> >> -Liran
> >>
> >>>
> >>>>>
> >>>>> --
> >>>>> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net v2] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-21 14:04 UTC (permalink / raw)
To: Si-Wei Liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, Alexander Duyck,
virtualization, liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <1551928112-32109-1-git-send-email-si-wei.liu@oracle.com>
On Wed, Mar 06, 2019 at 10:08:32PM -0500, Si-Wei Liu wrote:
> When a netdev appears through hot plug then gets enslaved by a failover
> master that is already up and running, the slave will be opened
> right away after getting enslaved. Today there's a race that userspace
> (udev) may fail to rename the slave if the kernel (net_failover)
> opens the slave earlier than when the userspace rename happens.
> Unlike bond or team, the primary slave of failover can't be renamed by
> userspace ahead of time, since the kernel initiated auto-enslavement is
> unable to, or rather, is never meant to be synchronized with the rename
> request from userspace.
>
> As the failover slave interfaces are not designed to be operated
> directly by userspace apps: IP configuration, filter rules with
> regard to network traffic passing and etc., should all be done on master
> interface. In general, userspace apps only care about the
> name of master interface, while slave names are less important as long
> as admin users can see reliable names that may carry
> other information describing the netdev. For e.g., they can infer that
> "ens3nsby" is a standby slave of "ens3", while for a
> name like "eth0" they can't tell which master it belongs to.
>
> Historically the name of IFF_UP interface can't be changed because
> there might be admin script or management software that is already
> relying on such behavior and assumes that the slave name can't be
> changed once UP. But failover is special: with the in-kernel
> auto-enslavement mechanism, the userspace expectation for device
> enumeration and bring-up order is already broken. Previously initramfs
> and various userspace config tools were modified to bypass failover
> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> in case that users care about seeing reliable slave name, the new type
> of failover slaves needs to be taken care of specifically in userspace
> anyway.
>
> It's less risky to lift up the rename restriction on failover slave
> which is already UP. Although it's possible this change may potentially
> break userspace component (most likely configuration scripts or
> management software) that assumes slave name can't be changed while
> UP, it's relatively a limited and controllable set among all userspace
> components, which can be fixed specifically to work with the new naming
> behavior of failover slaves. Userspace component interacting with
> slaves should be changed to operate on failover master instead, as the
> failover slave is dynamic in nature which may come and go at any point.
> The goal is to make the role of failover slaves less relevant, and
> all userspace should only deal with master in the long run.
>
> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
I worry that userspace might have made a bunch of assumptions
that names never change as long as interface is up.
So listening for up events ensures that interface
is not renamed.
How about sending down and up events around such renames?
> ---
> v1 -> v2:
> - Drop configurable module parameter (Sridhar)
>
>
> include/linux/netdevice.h | 3 +++
> net/core/dev.c | 3 ++-
> net/core/failover.c | 6 +++---
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 857f8ab..6d9e4e0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1487,6 +1487,7 @@ struct net_device_ops {
> * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> * @IFF_FAILOVER: device is a failover master device
> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> + * @IFF_SLAVE_RENAME_OK: rename is allowed while slave device is running
> */
> enum netdev_priv_flags {
> IFF_802_1Q_VLAN = 1<<0,
> @@ -1518,6 +1519,7 @@ enum netdev_priv_flags {
> IFF_NO_RX_HANDLER = 1<<26,
> IFF_FAILOVER = 1<<27,
> IFF_FAILOVER_SLAVE = 1<<28,
> + IFF_SLAVE_RENAME_OK = 1<<29,
> };
>
> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
> #define IFF_NO_RX_HANDLER IFF_NO_RX_HANDLER
> #define IFF_FAILOVER IFF_FAILOVER
> #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> +#define IFF_SLAVE_RENAME_OK IFF_SLAVE_RENAME_OK
>
> /**
> * struct net_device - The DEVICE structure.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 722d50d..ae070de 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1180,7 +1180,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
> BUG_ON(!dev_net(dev));
>
> net = dev_net(dev);
> - if (dev->flags & IFF_UP)
> + if (dev->flags & IFF_UP &&
> + !(dev->priv_flags & IFF_SLAVE_RENAME_OK))
> return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
> diff --git a/net/core/failover.c b/net/core/failover.c
> index 4a92a98..34c5c87 100644
> --- a/net/core/failover.c
> +++ b/net/core/failover.c
> @@ -80,14 +80,14 @@ static int failover_slave_register(struct net_device *slave_dev)
> goto err_upper_link;
> }
>
> - slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
> + slave_dev->priv_flags |= (IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
>
> if (fops && fops->slave_register &&
> !fops->slave_register(slave_dev, failover_dev))
> return NOTIFY_OK;
>
> netdev_upper_dev_unlink(slave_dev, failover_dev);
> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> err_upper_link:
> netdev_rx_handler_unregister(slave_dev);
> done:
> @@ -121,7 +121,7 @@ int failover_slave_unregister(struct net_device *slave_dev)
>
> netdev_rx_handler_unregister(slave_dev);
> netdev_upper_dev_unlink(slave_dev, failover_dev);
> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
>
> if (fops && fops->slave_unregister &&
> !fops->slave_unregister(slave_dev, failover_dev))
> --
> 1.8.3.1
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Liran Alon @ 2019-03-21 14:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321094217-mutt-send-email-mst@kernel.org>
> On 21 Mar 2019, at 15:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 21, 2019 at 03:24:39PM +0200, Liran Alon wrote:
>>
>>
>>> On 21 Mar 2019, at 15:12, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Mar 21, 2019 at 03:04:37PM +0200, Liran Alon wrote:
>>>>
>>>>
>>>>> On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>
>>>>> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
>>>>>>
>>>>>>
>>>>>>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>
>>>>>>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
>>>>>>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
>>>>>>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
>>>>>>>>>>>>
>>>>>>>>>>>> The set of changes I vision to fix our issues are:
>>>>>>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
>>>>>>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
>>>>>>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
>>>>>>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
>>>>>>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
>>>>>>>>>>>>
>>>>>>>>>>>> Is there any clear issue we see regarding the above suggestion?
>>>>>>>>>>>>
>>>>>>>>>>>> -Liran
>>>>>>>>>>>
>>>>>>>>>>> The issue would be this: how do we avoid conflicting with namespaces
>>>>>>>>>>> created by users?
>>>>>>>>>>
>>>>>>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
>>>>>>>>>> To reference a hidden netns, you need to do it explicitly.
>>>>>>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
>>>>>>>>>
>>>>>>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
>>>>>>>>
>>>>>>>> This is also a good idea that will solve the issue. Yes.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Does this seems reasonable?
>>>>>>>>>>
>>>>>>>>>> -Liran
>>>>>>>>>
>>>>>>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
>>>>>>>>> missed a trick or two.
>>>>>>>>
>>>>>>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
>>>>>>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
>>>>>>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
>>>>>>>> the breakage we have today for the various userspace components.
>>>>>>>>
>>>>>>>> -Liran
>>>>>>>
>>>>>>> It seems quite easy to supply that as a module parameter. Do we need two
>>>>>>> namespaces though? Won't some userspace still be confused by the two
>>>>>>> slaves sharing the MAC address?
>>>>>>
>>>>>> That’s one reasonable option.
>>>>>> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
>>>>>> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
>>>>>>
>>>>>> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
>>>>>>
>>>>>> -Liran
>>>>>
>>>>> OK. Now what happens if master is moved to another namespace? Do we need
>>>>> to move the slaves too?
>>>>
>>>> No. Why would we move the slaves?
>>>
>>>
>>> The reason we have 3 device model at all is so users can fine tune the
>>> slaves.
>>
>> I Agree.
>>
>>> I don't see why this applies to the root namespace but not
>>> a container. If it has access to failover it should have access
>>> to slaves.
>>
>> Oh now I see your point. I haven’t thought about the containers usage.
>> My thinking was that customer can always just enter to the “hidden” netns and configure there whatever he wants.
>>
>> Do you have a suggestion how to handle this?
>>
>> One option can be that every "visible" netns on system will have a “hidden” unnamed netns where the net-failover slaves reside in.
>> If customer wishes to be able to enter to that netns and manage the net-failover slaves explicitly, it will need to have an updated iproute2
>> that knows how to enter to that hidden netns. For most customers, they won’t need to ever enter that netns and thus it is ok they don’t
>> have this updated iproute2.
>
> Right so slaves need to be moved whenever master is moved.
>
> Given the amount of mess involved, should we just teach
> userspace to create the hidden netns and move slaves there?
That’s a good question.
However, I believe that it is easier and more suitable to happen in kernel. This is because:
1) Implementation is generic across all various distros.
2) We seem to discover more and more issues with userspace as we keep testing this on various distros, configurations and workloads.
3) It seems weird that kernel does some things automagically and some things don’t. i.e. Kernel automatically binds the virtio-net and VF to net-failover master
and automatically opens the net-failover slave when the net-failover master is opened, but it doesn’t care about the consequences these actions have on userspace.
Therefore, I propose let’s go “all in”: Kernel should also be responsible for hiding it’s artefacts unless customer userspace explicitly wants to view and manipulate them.
>
>>>
>>>> The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
>>>
>>> So that makes the common case easy. That is good. My worry is it might
>>> make some uncommon cases impossible.
>>>
>>>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>>>>
>>>>>
>>>>> Also siwei's patch is then kind of extraneous right?
>>>>> Attempts to rename a slave will now fail as it's in a namespace…
>>>>
>>>> I’m not sure actually. Isn't udev/systemd netns-aware?
>>>> I would expect it to be able to provide names also to netdevs in netns different than default netns.
>>>
>>> I think most people move devices after they are renamed.
>>
>> So?
>> Si-Wei patch handles the issue that resolves from the fact the net-failover master will be opened before the rename on the net-failover slaves occur.
>> This should happen (to my understanding) regardless of network namespaces.
>>
>> -Liran
>
> My point was that any tool that moves devices after they
> are renamed will be broken by kernel automatically putting
> them in a namespace.
I’m not sure I follow. How is this related to Si-Wei patch?
Si-Wei patch (and the root-cause that leads to the issue it fixes) have nothing to do with network namespaces.
What do you mean tool that moves devices after they are renamed will be broken by kernel?
Care to give an example to clarify?
-Liran
>
>>>
>>>> If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
>>>>
>>>> -Liran
>>>>
>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net v2] failover: allow name change on IFF_UP slave interfaces
From: Liran Alon @ 2019-03-21 14:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, Alexander Duyck,
virtualization, Netdev, Si-Wei Liu, boris.ostrovsky, David Miller
In-Reply-To: <20190321100110-mutt-send-email-mst@kernel.org>
> On 21 Mar 2019, at 16:04, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Mar 06, 2019 at 10:08:32PM -0500, Si-Wei Liu wrote:
>> When a netdev appears through hot plug then gets enslaved by a failover
>> master that is already up and running, the slave will be opened
>> right away after getting enslaved. Today there's a race that userspace
>> (udev) may fail to rename the slave if the kernel (net_failover)
>> opens the slave earlier than when the userspace rename happens.
>> Unlike bond or team, the primary slave of failover can't be renamed by
>> userspace ahead of time, since the kernel initiated auto-enslavement is
>> unable to, or rather, is never meant to be synchronized with the rename
>> request from userspace.
>>
>> As the failover slave interfaces are not designed to be operated
>> directly by userspace apps: IP configuration, filter rules with
>> regard to network traffic passing and etc., should all be done on master
>> interface. In general, userspace apps only care about the
>> name of master interface, while slave names are less important as long
>> as admin users can see reliable names that may carry
>> other information describing the netdev. For e.g., they can infer that
>> "ens3nsby" is a standby slave of "ens3", while for a
>> name like "eth0" they can't tell which master it belongs to.
>>
>> Historically the name of IFF_UP interface can't be changed because
>> there might be admin script or management software that is already
>> relying on such behavior and assumes that the slave name can't be
>> changed once UP. But failover is special: with the in-kernel
>> auto-enslavement mechanism, the userspace expectation for device
>> enumeration and bring-up order is already broken. Previously initramfs
>> and various userspace config tools were modified to bypass failover
>> slaves because of auto-enslavement and duplicate MAC address. Similarly,
>> in case that users care about seeing reliable slave name, the new type
>> of failover slaves needs to be taken care of specifically in userspace
>> anyway.
>>
>> It's less risky to lift up the rename restriction on failover slave
>> which is already UP. Although it's possible this change may potentially
>> break userspace component (most likely configuration scripts or
>> management software) that assumes slave name can't be changed while
>> UP, it's relatively a limited and controllable set among all userspace
>> components, which can be fixed specifically to work with the new naming
>> behavior of failover slaves. Userspace component interacting with
>> slaves should be changed to operate on failover master instead, as the
>> failover slave is dynamic in nature which may come and go at any point.
>> The goal is to make the role of failover slaves less relevant, and
>> all userspace should only deal with master in the long run.
>>
>> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> I worry that userspace might have made a bunch of assumptions
> that names never change as long as interface is up.
> So listening for up events ensures that interface
> is not renamed.
That’s true. This is exactly what is described in 3rd paragraph of commit message.
However, as commit message claims, net-failover slaves can be treated specially
because userspace is already broken on their handling and they need to be modified
to behave specially in regards to those slaves. Therefore, it’s less risky to lift up the
rename restriction on failover slave which is already UP.
>
> How about sending down and up events around such renames?
You mean that dev_change_name() will behave as proposed in this patch but also in addition
send fake DOWN and UP uevents to userspace?
-Liran
>
>
>
>> ---
>> v1 -> v2:
>> - Drop configurable module parameter (Sridhar)
>>
>>
>> include/linux/netdevice.h | 3 +++
>> net/core/dev.c | 3 ++-
>> net/core/failover.c | 6 +++---
>> 3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 857f8ab..6d9e4e0 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1487,6 +1487,7 @@ struct net_device_ops {
>> * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
>> * @IFF_FAILOVER: device is a failover master device
>> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
>> + * @IFF_SLAVE_RENAME_OK: rename is allowed while slave device is running
>> */
>> enum netdev_priv_flags {
>> IFF_802_1Q_VLAN = 1<<0,
>> @@ -1518,6 +1519,7 @@ enum netdev_priv_flags {
>> IFF_NO_RX_HANDLER = 1<<26,
>> IFF_FAILOVER = 1<<27,
>> IFF_FAILOVER_SLAVE = 1<<28,
>> + IFF_SLAVE_RENAME_OK = 1<<29,
>> };
>>
>> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
>> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
>> #define IFF_NO_RX_HANDLER IFF_NO_RX_HANDLER
>> #define IFF_FAILOVER IFF_FAILOVER
>> #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
>> +#define IFF_SLAVE_RENAME_OK IFF_SLAVE_RENAME_OK
>>
>> /**
>> * struct net_device - The DEVICE structure.
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 722d50d..ae070de 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1180,7 +1180,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
>> BUG_ON(!dev_net(dev));
>>
>> net = dev_net(dev);
>> - if (dev->flags & IFF_UP)
>> + if (dev->flags & IFF_UP &&
>> + !(dev->priv_flags & IFF_SLAVE_RENAME_OK))
>> return -EBUSY;
>>
>> write_seqcount_begin(&devnet_rename_seq);
>> diff --git a/net/core/failover.c b/net/core/failover.c
>> index 4a92a98..34c5c87 100644
>> --- a/net/core/failover.c
>> +++ b/net/core/failover.c
>> @@ -80,14 +80,14 @@ static int failover_slave_register(struct net_device *slave_dev)
>> goto err_upper_link;
>> }
>>
>> - slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
>> + slave_dev->priv_flags |= (IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
>>
>> if (fops && fops->slave_register &&
>> !fops->slave_register(slave_dev, failover_dev))
>> return NOTIFY_OK;
>>
>> netdev_upper_dev_unlink(slave_dev, failover_dev);
>> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
>> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
>> err_upper_link:
>> netdev_rx_handler_unregister(slave_dev);
>> done:
>> @@ -121,7 +121,7 @@ int failover_slave_unregister(struct net_device *slave_dev)
>>
>> netdev_rx_handler_unregister(slave_dev);
>> netdev_upper_dev_unlink(slave_dev, failover_dev);
>> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
>> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
>>
>> if (fops && fops->slave_unregister &&
>> !fops->slave_unregister(slave_dev, failover_dev))
>> --
>> 1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] VMCI/VSOCK: Add maintainers for VMCI, AF_VSOCK and VMCI transport
From: Jorgen Hansen via Virtualization @ 2019-03-21 14:41 UTC (permalink / raw)
To: linux-kernel, virtualization; +Cc: pv-drivers, gregkh, Jorgen Hansen
Update the maintainers file to include maintainers for the VMware
vmci driver, af_vsock, and the vsock vmci transport.
Signed-off-by: Jorgen Hansen <jhansen@vmware.com>
---
MAINTAINERS | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index e17ebf7..b9714fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16630,6 +16630,14 @@ S: Maintained
F: drivers/scsi/vmw_pvscsi.c
F: drivers/scsi/vmw_pvscsi.h
+VMWARE VMCI DRIVER
+M: Jorgen Hansen <jhansen@vmware.com>
+M: Vishnu Dasa <vdasa@vmware.com>
+M: "VMware, Inc." <pv-drivers@vmware.com>
+L: linux-kernel@vger.kernel.org
+S: Maintained
+F: drivers/misc/vmw_vmci/
+
VMWARE VMMOUSE SUBDRIVER
M: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
M: "VMware, Inc." <pv-drivers@vmware.com>
@@ -16638,6 +16646,18 @@ S: Maintained
F: drivers/input/mouse/vmmouse.c
F: drivers/input/mouse/vmmouse.h
+VMWARE VSOCK DRIVER (AF_VSOCK) AND VMCI TRANSPORT
+M: Jorgen Hansen <jhansen@vmware.com>
+M: Vishnu Dasa <vdasa@vmware.com>
+M: "VMware, Inc." <pv-drivers@vmware.com>
+L: netdev@vger.kernel.org
+S: Maintained
+F: net/vmw_vsock/af_vsock.c
+F: net/vmw_vsock/vmci_transport*
+F: net/vmw_vsock/vsock_addr.c
+F: include/linux/vm_sockets.h
+F: include/uapi/linux/vm_sockets.h
+
VMWARE VMXNET3 ETHERNET DRIVER
M: Ronak Doshi <doshir@vmware.com>
M: "VMware, Inc." <pv-drivers@vmware.com>
--
2.6.2
^ permalink raw reply related
* Re: [PATCH net v2] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-21 14:49 UTC (permalink / raw)
To: Liran Alon
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, Alexander Duyck,
virtualization, Netdev, Si-Wei Liu, boris.ostrovsky, David Miller
In-Reply-To: <00E221D7-FA96-4D12-8863-1D985F044968@oracle.com>
On Thu, Mar 21, 2019 at 04:20:40PM +0200, Liran Alon wrote:
>
>
> > On 21 Mar 2019, at 16:04, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Mar 06, 2019 at 10:08:32PM -0500, Si-Wei Liu wrote:
> >> When a netdev appears through hot plug then gets enslaved by a failover
> >> master that is already up and running, the slave will be opened
> >> right away after getting enslaved. Today there's a race that userspace
> >> (udev) may fail to rename the slave if the kernel (net_failover)
> >> opens the slave earlier than when the userspace rename happens.
> >> Unlike bond or team, the primary slave of failover can't be renamed by
> >> userspace ahead of time, since the kernel initiated auto-enslavement is
> >> unable to, or rather, is never meant to be synchronized with the rename
> >> request from userspace.
> >>
> >> As the failover slave interfaces are not designed to be operated
> >> directly by userspace apps: IP configuration, filter rules with
> >> regard to network traffic passing and etc., should all be done on master
> >> interface. In general, userspace apps only care about the
> >> name of master interface, while slave names are less important as long
> >> as admin users can see reliable names that may carry
> >> other information describing the netdev. For e.g., they can infer that
> >> "ens3nsby" is a standby slave of "ens3", while for a
> >> name like "eth0" they can't tell which master it belongs to.
> >>
> >> Historically the name of IFF_UP interface can't be changed because
> >> there might be admin script or management software that is already
> >> relying on such behavior and assumes that the slave name can't be
> >> changed once UP. But failover is special: with the in-kernel
> >> auto-enslavement mechanism, the userspace expectation for device
> >> enumeration and bring-up order is already broken. Previously initramfs
> >> and various userspace config tools were modified to bypass failover
> >> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> >> in case that users care about seeing reliable slave name, the new type
> >> of failover slaves needs to be taken care of specifically in userspace
> >> anyway.
> >>
> >> It's less risky to lift up the rename restriction on failover slave
> >> which is already UP. Although it's possible this change may potentially
> >> break userspace component (most likely configuration scripts or
> >> management software) that assumes slave name can't be changed while
> >> UP, it's relatively a limited and controllable set among all userspace
> >> components, which can be fixed specifically to work with the new naming
> >> behavior of failover slaves. Userspace component interacting with
> >> slaves should be changed to operate on failover master instead, as the
> >> failover slave is dynamic in nature which may come and go at any point.
> >> The goal is to make the role of failover slaves less relevant, and
> >> all userspace should only deal with master in the long run.
> >>
> >> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> >> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > I worry that userspace might have made a bunch of assumptions
> > that names never change as long as interface is up.
> > So listening for up events ensures that interface
> > is not renamed.
>
> That’s true. This is exactly what is described in 3rd paragraph of commit message.
> However, as commit message claims, net-failover slaves can be treated specially
> because userspace is already broken on their handling and they need to be modified
> to behave specially in regards to those slaves. Therefore, it’s less risky to lift up the
> rename restriction on failover slave which is already UP.
>
> >
> > How about sending down and up events around such renames?
>
> You mean that dev_change_name() will behave as proposed in this patch but also in addition
> send fake DOWN and UP uevents to userspace?
>
> -Liran
That was what I was trying to say.
> >
> >
> >
> >> ---
> >> v1 -> v2:
> >> - Drop configurable module parameter (Sridhar)
> >>
> >>
> >> include/linux/netdevice.h | 3 +++
> >> net/core/dev.c | 3 ++-
> >> net/core/failover.c | 6 +++---
> >> 3 files changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 857f8ab..6d9e4e0 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -1487,6 +1487,7 @@ struct net_device_ops {
> >> * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> >> * @IFF_FAILOVER: device is a failover master device
> >> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> >> + * @IFF_SLAVE_RENAME_OK: rename is allowed while slave device is running
> >> */
> >> enum netdev_priv_flags {
> >> IFF_802_1Q_VLAN = 1<<0,
> >> @@ -1518,6 +1519,7 @@ enum netdev_priv_flags {
> >> IFF_NO_RX_HANDLER = 1<<26,
> >> IFF_FAILOVER = 1<<27,
> >> IFF_FAILOVER_SLAVE = 1<<28,
> >> + IFF_SLAVE_RENAME_OK = 1<<29,
> >> };
> >>
> >> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> >> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
> >> #define IFF_NO_RX_HANDLER IFF_NO_RX_HANDLER
> >> #define IFF_FAILOVER IFF_FAILOVER
> >> #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> >> +#define IFF_SLAVE_RENAME_OK IFF_SLAVE_RENAME_OK
> >>
> >> /**
> >> * struct net_device - The DEVICE structure.
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 722d50d..ae070de 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -1180,7 +1180,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
> >> BUG_ON(!dev_net(dev));
> >>
> >> net = dev_net(dev);
> >> - if (dev->flags & IFF_UP)
> >> + if (dev->flags & IFF_UP &&
> >> + !(dev->priv_flags & IFF_SLAVE_RENAME_OK))
> >> return -EBUSY;
> >>
> >> write_seqcount_begin(&devnet_rename_seq);
> >> diff --git a/net/core/failover.c b/net/core/failover.c
> >> index 4a92a98..34c5c87 100644
> >> --- a/net/core/failover.c
> >> +++ b/net/core/failover.c
> >> @@ -80,14 +80,14 @@ static int failover_slave_register(struct net_device *slave_dev)
> >> goto err_upper_link;
> >> }
> >>
> >> - slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
> >> + slave_dev->priv_flags |= (IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> >>
> >> if (fops && fops->slave_register &&
> >> !fops->slave_register(slave_dev, failover_dev))
> >> return NOTIFY_OK;
> >>
> >> netdev_upper_dev_unlink(slave_dev, failover_dev);
> >> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> >> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> >> err_upper_link:
> >> netdev_rx_handler_unregister(slave_dev);
> >> done:
> >> @@ -121,7 +121,7 @@ int failover_slave_unregister(struct net_device *slave_dev)
> >>
> >> netdev_rx_handler_unregister(slave_dev);
> >> netdev_upper_dev_unlink(slave_dev, failover_dev);
> >> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> >> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> >>
> >> if (fops && fops->slave_unregister &&
> >> !fops->slave_unregister(slave_dev, failover_dev))
> >> --
> >> 1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-21 15:15 UTC (permalink / raw)
To: Liran Alon
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <FF25F82E-F6FF-424E-88BA-A3D74EE505DE@oracle.com>
On Thu, Mar 21, 2019 at 04:16:14PM +0200, Liran Alon wrote:
>
>
> > On 21 Mar 2019, at 15:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 03:24:39PM +0200, Liran Alon wrote:
> >>
> >>
> >>> On 21 Mar 2019, at 15:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>
> >>> On Thu, Mar 21, 2019 at 03:04:37PM +0200, Liran Alon wrote:
> >>>>
> >>>>
> >>>>> On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>
> >>>>> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
> >>>>>>
> >>>>>>
> >>>>>>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>>
> >>>>>>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
> >>>>>>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
> >>>>>>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The set of changes I vision to fix our issues are:
> >>>>>>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
> >>>>>>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
> >>>>>>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
> >>>>>>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
> >>>>>>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Is there any clear issue we see regarding the above suggestion?
> >>>>>>>>>>>>
> >>>>>>>>>>>> -Liran
> >>>>>>>>>>>
> >>>>>>>>>>> The issue would be this: how do we avoid conflicting with namespaces
> >>>>>>>>>>> created by users?
> >>>>>>>>>>
> >>>>>>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
> >>>>>>>>>> To reference a hidden netns, you need to do it explicitly.
> >>>>>>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
> >>>>>>>>>
> >>>>>>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
> >>>>>>>>
> >>>>>>>> This is also a good idea that will solve the issue. Yes.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Does this seems reasonable?
> >>>>>>>>>>
> >>>>>>>>>> -Liran
> >>>>>>>>>
> >>>>>>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
> >>>>>>>>> missed a trick or two.
> >>>>>>>>
> >>>>>>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
> >>>>>>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
> >>>>>>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
> >>>>>>>> the breakage we have today for the various userspace components.
> >>>>>>>>
> >>>>>>>> -Liran
> >>>>>>>
> >>>>>>> It seems quite easy to supply that as a module parameter. Do we need two
> >>>>>>> namespaces though? Won't some userspace still be confused by the two
> >>>>>>> slaves sharing the MAC address?
> >>>>>>
> >>>>>> That’s one reasonable option.
> >>>>>> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
> >>>>>> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
> >>>>>>
> >>>>>> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
> >>>>>>
> >>>>>> -Liran
> >>>>>
> >>>>> OK. Now what happens if master is moved to another namespace? Do we need
> >>>>> to move the slaves too?
> >>>>
> >>>> No. Why would we move the slaves?
> >>>
> >>>
> >>> The reason we have 3 device model at all is so users can fine tune the
> >>> slaves.
> >>
> >> I Agree.
> >>
> >>> I don't see why this applies to the root namespace but not
> >>> a container. If it has access to failover it should have access
> >>> to slaves.
> >>
> >> Oh now I see your point. I haven’t thought about the containers usage.
> >> My thinking was that customer can always just enter to the “hidden” netns and configure there whatever he wants.
> >>
> >> Do you have a suggestion how to handle this?
> >>
> >> One option can be that every "visible" netns on system will have a “hidden” unnamed netns where the net-failover slaves reside in.
> >> If customer wishes to be able to enter to that netns and manage the net-failover slaves explicitly, it will need to have an updated iproute2
> >> that knows how to enter to that hidden netns. For most customers, they won’t need to ever enter that netns and thus it is ok they don’t
> >> have this updated iproute2.
> >
> > Right so slaves need to be moved whenever master is moved.
> >
> > Given the amount of mess involved, should we just teach
> > userspace to create the hidden netns and move slaves there?
>
> That’s a good question.
>
> However, I believe that it is easier and more suitable to happen in kernel. This is because:
> 1) Implementation is generic across all various distros.
> 2) We seem to discover more and more issues with userspace as we keep testing this on various distros, configurations and workloads.
> 3) It seems weird that kernel does some things automagically and some things don’t. i.e. Kernel automatically binds the virtio-net and VF to net-failover master
> and automatically opens the net-failover slave when the net-failover master is opened, but it doesn’t care about the consequences these actions have on userspace.
> Therefore, I propose let’s go “all in”: Kernel should also be responsible for hiding it’s artefacts unless customer userspace explicitly wants to view and manipulate them.
Just a minor point: failover device is an artefact of kernel. Standy and
primary devices are created by the hypervisor.
> >
> >>>
> >>>> The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
> >>>
> >>> So that makes the common case easy. That is good. My worry is it might
> >>> make some uncommon cases impossible.
> >>>
> >>>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
> >>>>
> >>>>>
> >>>>> Also siwei's patch is then kind of extraneous right?
> >>>>> Attempts to rename a slave will now fail as it's in a namespace…
> >>>>
> >>>> I’m not sure actually. Isn't udev/systemd netns-aware?
> >>>> I would expect it to be able to provide names also to netdevs in netns different than default netns.
> >>>
> >>> I think most people move devices after they are renamed.
> >>
> >> So?
> >> Si-Wei patch handles the issue that resolves from the fact the net-failover master will be opened before the rename on the net-failover slaves occur.
> >> This should happen (to my understanding) regardless of network namespaces.
> >>
> >> -Liran
> >
> > My point was that any tool that moves devices after they
> > are renamed will be broken by kernel automatically putting
> > them in a namespace.
>
> I’m not sure I follow. How is this related to Si-Wei patch?
> Si-Wei patch (and the root-cause that leads to the issue it fixes) have nothing to do with network namespaces.
>
> What do you mean tool that moves devices after they are renamed will be broken by kernel?
> Care to give an example to clarify?
>
> -Liran
I'll have to get back to you next week when I'm less jetlaged and more
lucid.
> >
> >>>
> >>>> If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
> >>>>
> >>>> -Liran
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Stephen Hemminger @ 2019-03-21 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Liran Alon, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321085159-mutt-send-email-mst@kernel.org>
On Thu, 21 Mar 2019 08:57:03 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
> >
> >
> > > On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
> > >>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
> > >>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
> > >>>>>>
> > >>>>>> The set of changes I vision to fix our issues are:
> > >>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
> > >>>>>> (E.g. Configure the net-failover VF slave in some special way).
> > >>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
> > >>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
> > >>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
> > >>>>>>
> > >>>>>> Is there any clear issue we see regarding the above suggestion?
> > >>>>>>
> > >>>>>> -Liran
> > >>>>>
> > >>>>> The issue would be this: how do we avoid conflicting with namespaces
> > >>>>> created by users?
> > >>>>
> > >>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
> > >>>> To reference a hidden netns, you need to do it explicitly.
> > >>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
> > >>>
> > >>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
> > >>
> > >> This is also a good idea that will solve the issue. Yes.
> > >>
> > >>>
> > >>>> Does this seems reasonable?
> > >>>>
> > >>>> -Liran
> > >>>
> > >>> Reasonable I'd say yes, easy to implement probably no. But maybe I
> > >>> missed a trick or two.
> > >>
> > >> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
> > >> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
> > >> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
> > >> the breakage we have today for the various userspace components.
> > >>
> > >> -Liran
> > >
> > > It seems quite easy to supply that as a module parameter. Do we need two
> > > namespaces though? Won't some userspace still be confused by the two
> > > slaves sharing the MAC address?
> >
> > That’s one reasonable option.
> > Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
> > i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
> >
> > The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
> >
> > -Liran
>
> OK. Now what happens if master is moved to another namespace? Do we need
> to move the slaves too?
>
> Also siwei's patch is then kind of extraneous right?
> Attempts to rename a slave will now fail as it's in a namespace...
I did try moving slave device into a namespace at one point.
The problem is that introduces all sorts of locking problems in the code
because you can't do it directly in the context of when the callback
happens that a new slave device is discovered.
Since you can't safely change device namespace in the notifier,
it requires a work queue. Then you add more complexity and error cases
because the slave is exposed for a short period, and handling all the
state race unwinds...
Good idea but hard to implement
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Stephen Hemminger @ 2019-03-21 15:45 UTC (permalink / raw)
To: Liran Alon
Cc: vuhuong, Jiri Pirko, Michael S. Tsirkin, Jakub Kicinski,
Sridhar Samudrala, Alexander Duyck, virtualization, Netdev,
Si-Wei Liu, boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <2939FB15-720A-4C9E-92B7-2DBA139DDE0F@oracle.com>
On Thu, 21 Mar 2019 15:04:37 +0200
Liran Alon <liran.alon@oracle.com> wrote:
> >
> > OK. Now what happens if master is moved to another namespace? Do we need
> > to move the slaves too?
>
> No. Why would we move the slaves? The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
The 2-device netvsc already handles case where master changes namespace.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-21 15:50 UTC (permalink / raw)
To: Stephen Hemminger
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Liran Alon, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321084517.773a65fa@shemminger-XPS-13-9360>
On Thu, Mar 21, 2019 at 08:45:17AM -0700, Stephen Hemminger wrote:
> On Thu, 21 Mar 2019 15:04:37 +0200
> Liran Alon <liran.alon@oracle.com> wrote:
>
> > >
> > > OK. Now what happens if master is moved to another namespace? Do we need
> > > to move the slaves too?
> >
> > No. Why would we move the slaves? The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
> > We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>
>
> The 2-device netvsc already handles case where master changes namespace.
Is it by moving slave with it?
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Stefan Hajnoczi @ 2019-03-21 15:57 UTC (permalink / raw)
To: Jason Wang
Cc: axboe, mst, Cornelia Huck, linux-kernel, virtualization,
linux-block
In-Reply-To: <f92a5ef9-04b9-d6fa-a7f8-c855a87cd0fb@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 787 bytes --]
On Wed, Mar 20, 2019 at 08:53:33PM +0800, Jason Wang wrote:
>
> On 2019/3/19 上午10:22, Dongli Zhang wrote:
> > Hi Jason,
> >
> > On 3/18/19 3:47 PM, Jason Wang wrote:
> > > On 2019/3/15 下午8:41, Cornelia Huck wrote:
> > > > On Fri, 15 Mar 2019 12:50:11 +0800
> > > > Jason Wang <jasowang@redhat.com> wrote:
> > option 3:
> > We should allow more vectors even the block layer would support at most
> > nr_cpu_ids queues.
> >
> >
> > I understand a new policy for queue-vector mapping is very helpful. I am just
> > asking the question from block layer's point of view.
> >
> > Thank you very much!
> >
> > Dongli Zhang
>
>
> Don't know much for block, cc Stefan for more idea.
Thanks for CCing me. I don't have much input at this stage.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Liran Alon @ 2019-03-21 16:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321114557-mutt-send-email-mst@kernel.org>
> On 21 Mar 2019, at 17:50, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 21, 2019 at 08:45:17AM -0700, Stephen Hemminger wrote:
>> On Thu, 21 Mar 2019 15:04:37 +0200
>> Liran Alon <liran.alon@oracle.com> wrote:
>>
>>>>
>>>> OK. Now what happens if master is moved to another namespace? Do we need
>>>> to move the slaves too?
>>>
>>> No. Why would we move the slaves? The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
>>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>>
>>
>> The 2-device netvsc already handles case where master changes namespace.
>
> Is it by moving slave with it?
See c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device”).
It seems that when NetVSC master netdev changes netns, the VF is moved to the same netns by the NetVSC driver.
Kinda the opposite than what we are suggesting here to make sure that the net-failover master netdev is on a separate
netns than it’s slaves...
-Liran
>
> --
> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-21 17:12 UTC (permalink / raw)
To: Liran Alon
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <BC327EF2-22C7-402B-9078-93891A4774D6@oracle.com>
On Thu, Mar 21, 2019 at 06:31:35PM +0200, Liran Alon wrote:
>
>
> > On 21 Mar 2019, at 17:50, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 08:45:17AM -0700, Stephen Hemminger wrote:
> >> On Thu, 21 Mar 2019 15:04:37 +0200
> >> Liran Alon <liran.alon@oracle.com> wrote:
> >>
> >>>>
> >>>> OK. Now what happens if master is moved to another namespace? Do we need
> >>>> to move the slaves too?
> >>>
> >>> No. Why would we move the slaves? The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
> >>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
> >>
> >>
> >> The 2-device netvsc already handles case where master changes namespace.
> >
> > Is it by moving slave with it?
>
> See c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device”).
> It seems that when NetVSC master netdev changes netns, the VF is moved to the same netns by the NetVSC driver.
> Kinda the opposite than what we are suggesting here to make sure that the net-failover master netdev is on a separate
> netns than it’s slaves...
>
> -Liran
>
> >
> > --
> > MST
Not exactly opposite I'd say.
If failover is in host ns, slaves in /primary and /standby, then moving
failover to /container should move slaves to /container/primary and
/container/standby.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [summary] virtio network device failover writeup
From: Liran Alon @ 2019-03-21 17:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
Alexander Duyck, virtualization, Netdev, Si-Wei Liu,
boris.ostrovsky, David Miller, ogerlitz
In-Reply-To: <20190321131006-mutt-send-email-mst@kernel.org>
> On 21 Mar 2019, at 19:12, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 21, 2019 at 06:31:35PM +0200, Liran Alon wrote:
>>
>>
>>> On 21 Mar 2019, at 17:50, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Mar 21, 2019 at 08:45:17AM -0700, Stephen Hemminger wrote:
>>>> On Thu, 21 Mar 2019 15:04:37 +0200
>>>> Liran Alon <liran.alon@oracle.com> wrote:
>>>>
>>>>>>
>>>>>> OK. Now what happens if master is moved to another namespace? Do we need
>>>>>> to move the slaves too?
>>>>>
>>>>> No. Why would we move the slaves? The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
>>>>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
>>>>
>>>>
>>>> The 2-device netvsc already handles case where master changes namespace.
>>>
>>> Is it by moving slave with it?
>>
>> See c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device”).
>> It seems that when NetVSC master netdev changes netns, the VF is moved to the same netns by the NetVSC driver.
>> Kinda the opposite than what we are suggesting here to make sure that the net-failover master netdev is on a separate
>> netns than it’s slaves...
>>
>> -Liran
>>
>>>
>>> --
>>> MST
>
> Not exactly opposite I'd say.
>
> If failover is in host ns, slaves in /primary and /standby, then moving
> failover to /container should move slaves to /container/primary and
> /container/standby.
Yes I agree.
I meant that they tried to keep the VF on the same netns as the NetVSC.
But of course what you just described is exactly the functionality I would have wanted in our net-failover mechanism.
-Liran
>
>
> --
> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-03-22 0:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Jean-Philippe Brucker, Benjamin Herrenschmidt,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
Paul Mackerras, iommu, linuxppc-dev, Christoph Hellwig,
David Gibson
In-Reply-To: <20190320171027-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin <mst@redhat.com> writes:
> On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
>> >> Another way of looking at this issue which also explains our reluctance
>> >> is that the only difference between a secure guest and a regular guest
>> >> (at least regarding virtio) is that the former uses swiotlb while the
>> >> latter doens't.
>> >
>> > But swiotlb is just one implementation. It's a guest internal thing. The
>> > issue is that memory isn't host accessible.
>>
>> >From what I understand of the ACCESS_PLATFORM definition, the host will
>> only ever try to access memory addresses that are supplied to it by the
>> guest, so all of the secure guest memory that the host cares about is
>> accessible:
>>
>> If this feature bit is set to 0, then the device has same access to
>> memory addresses supplied to it as the driver has. In particular,
>> the device will always use physical addresses matching addresses
>> used by the driver (typically meaning physical addresses used by the
>> CPU) and not translated further, and can access any address supplied
>> to it by the driver. When clear, this overrides any
>> platform-specific description of whether device access is limited or
>> translated in any way, e.g. whether an IOMMU may be present.
>>
>> All of the above is true for POWER guests, whether they are secure
>> guests or not.
>>
>> Or are you saying that a virtio device may want to access memory
>> addresses that weren't supplied to it by the driver?
>
> Your logic would apply to IOMMUs as well. For your mode, there are
> specific encrypted memory regions that driver has access to but device
> does not. that seems to violate the constraint.
Right, if there's a pre-configured 1:1 mapping in the IOMMU such that
the device can ignore the IOMMU for all practical purposes I would
indeed say that the logic would apply to IOMMUs as well. :-)
I guess I'm still struggling with the purpose of signalling to the
driver that the host may not have access to memory addresses that it
will never try to access.
>> >> And from the device's point of view they're
>> >> indistinguishable. It can't tell one guest that is using swiotlb from
>> >> one that isn't. And that implies that secure guest vs regular guest
>> >> isn't a virtio interface issue, it's "guest internal affairs". So
>> >> there's no reason to reflect that in the feature flags.
>> >
>> > So don't. The way not to reflect that in the feature flags is
>> > to set ACCESS_PLATFORM. Then you say *I don't care let platform device*.
>> >
>> >
>> > Without ACCESS_PLATFORM
>> > virtio has a very specific opinion about the security of the
>> > device, and that opinion is that device is part of the guest
>> > supervisor security domain.
>>
>> Sorry for being a bit dense, but not sure what "the device is part of
>> the guest supervisor security domain" means. In powerpc-speak,
>> "supervisor" is the operating system so perhaps that explains my
>> confusion. Are you saying that without ACCESS_PLATFORM, the guest
>> considers the host to be part of the guest operating system's security
>> domain?
>
> I think so. The spec says "device has same access as driver".
Ok, makes sense.
>> If so, does that have any other implication besides "the host
>> can access any address supplied to it by the driver"? If that is the
>> case, perhaps the definition of ACCESS_PLATFORM needs to be amended to
>> include that information because it's not part of the current
>> definition.
>>
>> >> > But the name "sev_active" makes me scared because at least AMD guys who
>> >> > were doing the sensible thing and setting ACCESS_PLATFORM
>> >>
>> >> My understanding is, AMD guest-platform knows in advance that their
>> >> guest will run in secure mode and hence sets the flag at the time of VM
>> >> instantiation. Unfortunately we dont have that luxury on our platforms.
>> >
>> > Well you do have that luxury. It looks like that there are existing
>> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
>> > with how that path is slow. So you are trying to optimize for
>> > them by clearing ACCESS_PLATFORM and then you have lost ability
>> > to invoke DMA API.
>> >
>> > For example if there was another flag just like ACCESS_PLATFORM
>> > just not yet used by anyone, you would be all fine using that right?
>>
>> Yes, a new flag sounds like a great idea. What about the definition
>> below?
>>
>> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
>> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
>> exception that the IOMMU is explicitly defined to be off or bypassed
>> when accessing memory addresses supplied to the device by the
>> driver. This flag should be set by the guest if offered, but to
>> allow for backward-compatibility device implementations allow for it
>> to be left unset by the guest. It is an error to set both this flag
>> and VIRTIO_F_ACCESS_PLATFORM.
>
> It looks kind of narrow but it's an option.
Great!
> I wonder how we'll define what's an iommu though.
Hm, it didn't occur to me it could be an issue. I'll try.
> Another idea is maybe something like virtio-iommu?
You mean, have legacy guests use virtio-iommu to request an IOMMU
bypass? If so, it's an interesting idea for new guests but it doesn't
help with guests that are out today in the field, which don't have A
virtio-iommu driver.
>> > Is there any justification to doing that beyond someone putting
>> > out slow code in the past?
>>
>> The definition of the ACCESS_PLATFORM flag is generic and captures the
>> notion of memory access restrictions for the device. Unfortunately, on
>> powerpc pSeries guests it also implies that the IOMMU is turned on
>
> IIUC that's really because on pSeries IOMMU is *always* turned on.
> Platform has no way to say what you want it to say
> which is bypass the iommu for the specific device.
Yes, that's correct. pSeries guests running on KVM are in a gray area
where theoretically they use an IOMMU but in practice KVM ignores it.
It's unfortunate but it's the reality on the ground today. :-/
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-03-23 21:01 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Mike Anderson, Jean-Philippe Brucker, Benjamin Herrenschmidt,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
Paul Mackerras, iommu, linuxppc-dev, Christoph Hellwig,
David Gibson
In-Reply-To: <87tvfvbwpb.fsf@morokweng.localdomain>
On Thu, Mar 21, 2019 at 09:05:04PM -0300, Thiago Jung Bauermann wrote:
>
> Michael S. Tsirkin <mst@redhat.com> writes:
>
> > On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
> >> >> Another way of looking at this issue which also explains our reluctance
> >> >> is that the only difference between a secure guest and a regular guest
> >> >> (at least regarding virtio) is that the former uses swiotlb while the
> >> >> latter doens't.
> >> >
> >> > But swiotlb is just one implementation. It's a guest internal thing. The
> >> > issue is that memory isn't host accessible.
> >>
> >> >From what I understand of the ACCESS_PLATFORM definition, the host will
> >> only ever try to access memory addresses that are supplied to it by the
> >> guest, so all of the secure guest memory that the host cares about is
> >> accessible:
> >>
> >> If this feature bit is set to 0, then the device has same access to
> >> memory addresses supplied to it as the driver has. In particular,
> >> the device will always use physical addresses matching addresses
> >> used by the driver (typically meaning physical addresses used by the
> >> CPU) and not translated further, and can access any address supplied
> >> to it by the driver. When clear, this overrides any
> >> platform-specific description of whether device access is limited or
> >> translated in any way, e.g. whether an IOMMU may be present.
> >>
> >> All of the above is true for POWER guests, whether they are secure
> >> guests or not.
> >>
> >> Or are you saying that a virtio device may want to access memory
> >> addresses that weren't supplied to it by the driver?
> >
> > Your logic would apply to IOMMUs as well. For your mode, there are
> > specific encrypted memory regions that driver has access to but device
> > does not. that seems to violate the constraint.
>
> Right, if there's a pre-configured 1:1 mapping in the IOMMU such that
> the device can ignore the IOMMU for all practical purposes I would
> indeed say that the logic would apply to IOMMUs as well. :-)
>
> I guess I'm still struggling with the purpose of signalling to the
> driver that the host may not have access to memory addresses that it
> will never try to access.
For example, one of the benefits is to signal to host that driver does
not expect ability to access all memory. If it does, host can
fail initialization gracefully.
> >> >> And from the device's point of view they're
> >> >> indistinguishable. It can't tell one guest that is using swiotlb from
> >> >> one that isn't. And that implies that secure guest vs regular guest
> >> >> isn't a virtio interface issue, it's "guest internal affairs". So
> >> >> there's no reason to reflect that in the feature flags.
> >> >
> >> > So don't. The way not to reflect that in the feature flags is
> >> > to set ACCESS_PLATFORM. Then you say *I don't care let platform device*.
> >> >
> >> >
> >> > Without ACCESS_PLATFORM
> >> > virtio has a very specific opinion about the security of the
> >> > device, and that opinion is that device is part of the guest
> >> > supervisor security domain.
> >>
> >> Sorry for being a bit dense, but not sure what "the device is part of
> >> the guest supervisor security domain" means. In powerpc-speak,
> >> "supervisor" is the operating system so perhaps that explains my
> >> confusion. Are you saying that without ACCESS_PLATFORM, the guest
> >> considers the host to be part of the guest operating system's security
> >> domain?
> >
> > I think so. The spec says "device has same access as driver".
>
> Ok, makes sense.
>
> >> If so, does that have any other implication besides "the host
> >> can access any address supplied to it by the driver"? If that is the
> >> case, perhaps the definition of ACCESS_PLATFORM needs to be amended to
> >> include that information because it's not part of the current
> >> definition.
> >>
> >> >> > But the name "sev_active" makes me scared because at least AMD guys who
> >> >> > were doing the sensible thing and setting ACCESS_PLATFORM
> >> >>
> >> >> My understanding is, AMD guest-platform knows in advance that their
> >> >> guest will run in secure mode and hence sets the flag at the time of VM
> >> >> instantiation. Unfortunately we dont have that luxury on our platforms.
> >> >
> >> > Well you do have that luxury. It looks like that there are existing
> >> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
> >> > with how that path is slow. So you are trying to optimize for
> >> > them by clearing ACCESS_PLATFORM and then you have lost ability
> >> > to invoke DMA API.
> >> >
> >> > For example if there was another flag just like ACCESS_PLATFORM
> >> > just not yet used by anyone, you would be all fine using that right?
> >>
> >> Yes, a new flag sounds like a great idea. What about the definition
> >> below?
> >>
> >> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
> >> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
> >> exception that the IOMMU is explicitly defined to be off or bypassed
> >> when accessing memory addresses supplied to the device by the
> >> driver. This flag should be set by the guest if offered, but to
> >> allow for backward-compatibility device implementations allow for it
> >> to be left unset by the guest. It is an error to set both this flag
> >> and VIRTIO_F_ACCESS_PLATFORM.
> >
> > It looks kind of narrow but it's an option.
>
> Great!
>
> > I wonder how we'll define what's an iommu though.
>
> Hm, it didn't occur to me it could be an issue. I'll try.
>
> > Another idea is maybe something like virtio-iommu?
>
> You mean, have legacy guests use virtio-iommu to request an IOMMU
> bypass? If so, it's an interesting idea for new guests but it doesn't
> help with guests that are out today in the field, which don't have A
> virtio-iommu driver.
I presume legacy guests don't use encrypted memory so why do we
worry about them at all?
> >> > Is there any justification to doing that beyond someone putting
> >> > out slow code in the past?
> >>
> >> The definition of the ACCESS_PLATFORM flag is generic and captures the
> >> notion of memory access restrictions for the device. Unfortunately, on
> >> powerpc pSeries guests it also implies that the IOMMU is turned on
> >
> > IIUC that's really because on pSeries IOMMU is *always* turned on.
> > Platform has no way to say what you want it to say
> > which is bypass the iommu for the specific device.
>
> Yes, that's correct. pSeries guests running on KVM are in a gray area
> where theoretically they use an IOMMU but in practice KVM ignores it.
> It's unfortunate but it's the reality on the ground today. :-/
Well it's not just the reality, virt setups need something that
emulated IOMMUs don't provide. That is not uncommon, e.g.
intel's VTD has a "cache mode" field which AFAIK is only used for virt.
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: David Gibson @ 2019-03-25 0:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Jean-Philippe Brucker, Benjamin Herrenschmidt,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
Paul Mackerras, iommu, linuxppc-dev, Christoph Hellwig
In-Reply-To: <20190323165456-mutt-send-email-mst@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 1684 bytes --]
On Sat, Mar 23, 2019 at 05:01:35PM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2019 at 09:05:04PM -0300, Thiago Jung Bauermann wrote:
> > Michael S. Tsirkin <mst@redhat.com> writes:
[snip]
> > >> > Is there any justification to doing that beyond someone putting
> > >> > out slow code in the past?
> > >>
> > >> The definition of the ACCESS_PLATFORM flag is generic and captures the
> > >> notion of memory access restrictions for the device. Unfortunately, on
> > >> powerpc pSeries guests it also implies that the IOMMU is turned on
> > >
> > > IIUC that's really because on pSeries IOMMU is *always* turned on.
> > > Platform has no way to say what you want it to say
> > > which is bypass the iommu for the specific device.
> >
> > Yes, that's correct. pSeries guests running on KVM are in a gray area
> > where theoretically they use an IOMMU but in practice KVM ignores it.
> > It's unfortunate but it's the reality on the ground today. :-/
Um.. I'm not sure what you mean by this. As far as I'm concerned
there is always a guest-visible (paravirtualized) IOMMU, and that will
be backed onto the host IOMMU when necessary.
[Actually there is an IOMMU bypass hack that's used by the guest
firmware, but I don't think we want to expose that]
> Well it's not just the reality, virt setups need something that
> emulated IOMMUs don't provide. That is not uncommon, e.g.
> intel's VTD has a "cache mode" field which AFAIK is only used for virt.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH -next] drm/ttm: remove set but not used variable 'vgdev'
From: Daniel Vetter @ 2019-03-25 9:02 UTC (permalink / raw)
To: YueHaibing
Cc: David Airlie, kernel-janitors, linux-kernel, dri-devel,
virtualization, Daniel Vetter
In-Reply-To: <20190320020308.12745-1-yuehaibing@huawei.com>
On Wed, Mar 20, 2019 at 02:03:08AM +0000, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/gpu/drm/virtio/virtgpu_ttm.c: In function 'virtio_gpu_init_mem_type':
> drivers/gpu/drm/virtio/virtgpu_ttm.c:117:28: warning:
> variable 'vgdev' set but not used [-Wunused-but-set-variable]
>
> drivers/gpu/drm/virtio/virtgpu_ttm.c: In function 'virtio_gpu_bo_swap_notify':
> drivers/gpu/drm/virtio/virtgpu_ttm.c:300:28: warning:
> variable 'vgdev' set but not used [-Wunused-but-set-variable]
>
> It is never used since introduction in dc5698e80cf7 ("Add virtio gpu driver.")
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Subject prefix should be drm/virtio: which is maybe the reason Gerd hasn't
spotted this yet.
-Daniel
> ---
> drivers/gpu/drm/virtio/virtgpu_ttm.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> index d6225ba20b30..eb007c2569d8 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> @@ -114,10 +114,6 @@ static const struct ttm_mem_type_manager_func virtio_gpu_bo_manager_func = {
> static int virtio_gpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> struct ttm_mem_type_manager *man)
> {
> - struct virtio_gpu_device *vgdev;
> -
> - vgdev = virtio_gpu_get_vgdev(bdev);
> -
> switch (type) {
> case TTM_PL_SYSTEM:
> /* System memory */
> @@ -297,10 +293,8 @@ static void virtio_gpu_bo_move_notify(struct ttm_buffer_object *tbo,
> static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
> {
> struct virtio_gpu_object *bo;
> - struct virtio_gpu_device *vgdev;
>
> bo = container_of(tbo, struct virtio_gpu_object, tbo);
> - vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
>
> if (bo->pages)
> virtio_gpu_object_free_sg_table(bo);
>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v2 -next] drm/virtio: remove set but not used variable 'vgdev'
From: Mukesh Ojha @ 2019-03-25 11:35 UTC (permalink / raw)
To: YueHaibing, David Airlie, Gerd Hoffmann, Daniel Vetter
Cc: kernel-janitors, linux-kernel, dri-devel, virtualization
In-Reply-To: <20190325092631.152060-1-yuehaibing@huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 1926 bytes --]
On 3/25/2019 2:56 PM, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/gpu/drm/virtio/virtgpu_ttm.c: In function 'virtio_gpu_init_mem_type':
> drivers/gpu/drm/virtio/virtgpu_ttm.c:117:28: warning:
> variable 'vgdev' set but not used [-Wunused-but-set-variable]
>
> drivers/gpu/drm/virtio/virtgpu_ttm.c: In function 'virtio_gpu_bo_swap_notify':
> drivers/gpu/drm/virtio/virtgpu_ttm.c:300:28: warning:
> variable 'vgdev' set but not used [-Wunused-but-set-variable]
>
> It is never used since introduction in dc5698e80cf7 ("Add virtio gpu driver.")
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org
<mailto:mojha@codeaurora.org>>
Thanks.
Mukesh
> ---
> v2: fix patch prefix
> ---
> drivers/gpu/drm/virtio/virtgpu_ttm.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> index d6225ba20b30..eb007c2569d8 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> @@ -114,10 +114,6 @@ static const struct ttm_mem_type_manager_func virtio_gpu_bo_manager_func = {
> static int virtio_gpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> struct ttm_mem_type_manager *man)
> {
> - struct virtio_gpu_device *vgdev;
> -
> - vgdev = virtio_gpu_get_vgdev(bdev);
> -
> switch (type) {
> case TTM_PL_SYSTEM:
> /* System memory */
> @@ -297,10 +293,8 @@ static void virtio_gpu_bo_move_notify(struct ttm_buffer_object *tbo,
> static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
> {
> struct virtio_gpu_object *bo;
> - struct virtio_gpu_device *vgdev;
>
> bo = container_of(tbo, struct virtio_gpu_object, tbo);
> - vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
>
> if (bo->pages)
> virtio_gpu_object_free_sg_table(bo);
>
>
>
[-- Attachment #1.2: Type: text/html, Size: 2592 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: INFO: task hung in vhost_net_stop_vq
From: Michael S. Tsirkin @ 2019-03-25 14:02 UTC (permalink / raw)
To: syzbot; +Cc: weiyj.lk, kvm, netdev, syzkaller-bugs, linux-kernel,
virtualization
In-Reply-To: <0000000000008ee87005847fc985@google.com>
Looks like more iotlb locking mess?
On Tue, Mar 19, 2019 at 10:21:00PM -0700, syzbot wrote:
> syzbot has bisected this bug to:
>
> commit 6b1e6cc7855b09a0a9bfa1d9f30172ba366f161c
> Author: Jason Wang <jasowang@redhat.com>
> Date: Thu Jun 23 06:04:32 2016 +0000
>
> vhost: new device IOTLB API
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1486ad27200000
> start commit: 6b1e6cc7 vhost: new device IOTLB API
> git tree: upstream
> final crash: https://syzkaller.appspot.com/x/report.txt?x=1686ad27200000
> console output: https://syzkaller.appspot.com/x/log.txt?x=1286ad27200000
> kernel config: https://syzkaller.appspot.com/x/.config?x=c94f9f0c0363db4b
> dashboard link: https://syzkaller.appspot.com/bug?extid=d21e6e297322a900c128
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=141db34d400000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=108ef293400000
>
> Reported-by: syzbot+d21e6e297322a900c128@syzkaller.appspotmail.com
> Fixes: 6b1e6cc7 ("vhost: new device IOTLB API")
^ permalink raw reply
* [PATCH] x86/paravirt: Guard against invalid cpu # in pv_vcpu_is_preempted()
From: Waiman Long @ 2019-03-25 15:57 UTC (permalink / raw)
To: Juergen Gross, Alok Kataria, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, Peter Zijlstra, Will Deacon
Cc: Waiman Long, Paolo Bonzini, x86, linux-kernel, virtualization
It was found that passing an invalid cpu number to pv_vcpu_is_preempted()
might panic the kernel in a VM guest. For example,
[ 2.531077] Oops: 0000 [#1] SMP PTI
:
[ 2.532545] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[ 2.533321] RIP: 0010:__raw_callee_save___kvm_vcpu_is_preempted+0x0/0x20
To guard against this kind of kernel panic, check is added to
pv_vcpu_is_preempted() to make sure that no invalid cpu number will
be used.
Signed-off-by: Waiman Long <longman@redhat.com>
---
arch/x86/include/asm/paravirt.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index c25c38a05c1c..4cfb465dcde4 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -671,6 +671,12 @@ static __always_inline void pv_kick(int cpu)
static __always_inline bool pv_vcpu_is_preempted(long cpu)
{
+ /*
+ * Guard against invalid cpu number or the kernel might panic.
+ */
+ if (WARN_ON_ONCE((unsigned long)cpu >= nr_cpu_ids))
+ return false;
+
return PVOP_CALLEE1(bool, lock.vcpu_is_preempted, cpu);
}
--
2.18.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