Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Fri, May 16, 2025 at 6:39 PM Michael S. Tsirkin wrote: > > On Fri, May 16, 2025 at 09:31:42AM +0800, Jason Wang wrote: > > On Thu, May 15, 2025 at 2:14 PM Michael S. Tsirkin wrote: > > > > > > On Wed, May 14, 2025 at 10:52:58AM +0800, Jason Wang wrote: > > > > On Tue, May 13, 2025 at 3:09 PM Michael S. Tsirkin > > > > wrote: > > > > > > > > > > On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > > > > > > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin > > > > > > wrote: > > > > > > > > > > > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > > > > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > > > > > > to control the availability of the > > > > > > > > > > > > > `VHOST_FORK_FROM_OWNER` ioctl. > > > > > > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to > > > > > > > > > > > > > n, the ioctl > > > > > > > > > > > > > is disabled, and any attempt to use it will result in > > > > > > > > > > > > > failure. > > > > > > > > > > > > > > > > > > > > > > > > I think we need to describe why the default value was > > > > > > > > > > > > chosen to be false. > > > > > > > > > > > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" > > > > > > > > > > > > userspace may > > > > > > > > > > > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise > > > > > > > > > > legacy > > > > > > > > > > applications need to be modified in order to get the > > > > > > > > > > behaviour > > > > > > > > > > recovered which is an impossible taks. > > > > > > > > > > > > > > > > > > > > Any idea on this? > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > So, let's say we had a modparam? Enough for this customer? > > > > > > > WDYT? > > > > > > > > > > > > Just to make sure I understand the proposal. > > > > > > > > > > > > Did you mean a module parameter like "inherit_owner_by_default"? I > > > > > > think it would be fine if we make it false by default. > > > > > > > > > > > > Thanks > > > > > > > > > > I think we should keep it true by default, changing the default > > > > > risks regressing what we already fixes. > > > > > > > > I think it's not a regression since it comes since the day vhost is > > > > introduced. To my understanding the real regression is the user space > > > > noticeable behaviour changes introduced by vhost thread. > > > > > > > > > The specific customer can > > > > > flip the modparam and be happy. > > > > > > > > If you stick to the false as default, I'm fine. > > > > > > > > Thanks > > > > > > That would be yet another behaviour change. > > > > Back to the original behaviour. > > yes but the original was also a bugfix. > > > > I think one was enough, don't you think? > > > > I think such kind of change is unavoidable if we want to fix the > > usersapce behaviour change. > > > > Thanks > > I feel it is too late to "fix". the new behaviour is generally ok, and I > feel the right thing so to give management control knobs do pick the > desired behaviour. > And really modparam is wrong here because different userspace > can have different requirements, and in ~10 years I want to see us > disable the legacy behaviour altogether. > But given your time constraints, a modparam knob as a quick workaround > for the specific customer is kind of not very terrible. > Ok, that makes sense. Thanks
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Fri, May 16, 2025 at 09:31:42AM +0800, Jason Wang wrote: > On Thu, May 15, 2025 at 2:14 PM Michael S. Tsirkin wrote: > > > > On Wed, May 14, 2025 at 10:52:58AM +0800, Jason Wang wrote: > > > On Tue, May 13, 2025 at 3:09 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > > > > > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin > > > > > wrote: > > > > > > > > > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > > > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > > > > > to control the availability of the > > > > > > > > > > > > `VHOST_FORK_FROM_OWNER` ioctl. > > > > > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, > > > > > > > > > > > > the ioctl > > > > > > > > > > > > is disabled, and any attempt to use it will result in > > > > > > > > > > > > failure. > > > > > > > > > > > > > > > > > > > > > > I think we need to describe why the default value was > > > > > > > > > > > chosen to be false. > > > > > > > > > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" > > > > > > > > > > > userspace may > > > > > > > > > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise > > > > > > > > > legacy > > > > > > > > > applications need to be modified in order to get the behaviour > > > > > > > > > recovered which is an impossible taks. > > > > > > > > > > > > > > > > > > Any idea on this? > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > So, let's say we had a modparam? Enough for this customer? > > > > > > WDYT? > > > > > > > > > > Just to make sure I understand the proposal. > > > > > > > > > > Did you mean a module parameter like "inherit_owner_by_default"? I > > > > > think it would be fine if we make it false by default. > > > > > > > > > > Thanks > > > > > > > > I think we should keep it true by default, changing the default > > > > risks regressing what we already fixes. > > > > > > I think it's not a regression since it comes since the day vhost is > > > introduced. To my understanding the real regression is the user space > > > noticeable behaviour changes introduced by vhost thread. > > > > > > > The specific customer can > > > > flip the modparam and be happy. > > > > > > If you stick to the false as default, I'm fine. > > > > > > Thanks > > > > That would be yet another behaviour change. > > Back to the original behaviour. yes but the original was also a bugfix. > > I think one was enough, don't you think? > > I think such kind of change is unavoidable if we want to fix the > usersapce behaviour change. > > Thanks I feel it is too late to "fix". the new behaviour is generally ok, and I feel the right thing so to give management control knobs do pick the desired behaviour. And really modparam is wrong here because different userspace can have different requirements, and in ~10 years I want to see us disable the legacy behaviour altogether. But given your time constraints, a modparam knob as a quick workaround for the specific customer is kind of not very terrible. > > > > > > > > > > > > > > > > > > > > -- > > > > > > MST > > > > > > > > > > > >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Thu, May 15, 2025 at 2:14 PM Michael S. Tsirkin wrote: > > On Wed, May 14, 2025 at 10:52:58AM +0800, Jason Wang wrote: > > On Tue, May 13, 2025 at 3:09 PM Michael S. Tsirkin wrote: > > > > > > On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > > > > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin > > > > wrote: > > > > > > > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > > > > to control the availability of the > > > > > > > > > > > `VHOST_FORK_FROM_OWNER` ioctl. > > > > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, > > > > > > > > > > > the ioctl > > > > > > > > > > > is disabled, and any attempt to use it will result in > > > > > > > > > > > failure. > > > > > > > > > > > > > > > > > > > > I think we need to describe why the default value was > > > > > > > > > > chosen to be false. > > > > > > > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" > > > > > > > > > > userspace may > > > > > > > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > > > > > > applications need to be modified in order to get the behaviour > > > > > > > > recovered which is an impossible taks. > > > > > > > > > > > > > > > > Any idea on this? > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > So, let's say we had a modparam? Enough for this customer? > > > > > WDYT? > > > > > > > > Just to make sure I understand the proposal. > > > > > > > > Did you mean a module parameter like "inherit_owner_by_default"? I > > > > think it would be fine if we make it false by default. > > > > > > > > Thanks > > > > > > I think we should keep it true by default, changing the default > > > risks regressing what we already fixes. > > > > I think it's not a regression since it comes since the day vhost is > > introduced. To my understanding the real regression is the user space > > noticeable behaviour changes introduced by vhost thread. > > > > > The specific customer can > > > flip the modparam and be happy. > > > > If you stick to the false as default, I'm fine. > > > > Thanks > > That would be yet another behaviour change. Back to the original behaviour. > I think one was enough, don't you think? I think such kind of change is unavoidable if we want to fix the usersapce behaviour change. Thanks > > > > > > > > > > > > > > > -- > > > > > MST > > > > > > > > >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Wed, May 14, 2025 at 10:52:58AM +0800, Jason Wang wrote: > On Tue, May 13, 2025 at 3:09 PM Michael S. Tsirkin wrote: > > > > On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > > > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > > > wrote: > > > > > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > > > wrote: > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` > > > > > > > > > > ioctl. > > > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the > > > > > > > > > > ioctl > > > > > > > > > > is disabled, and any attempt to use it will result in > > > > > > > > > > failure. > > > > > > > > > > > > > > > > > > I think we need to describe why the default value was chosen > > > > > > > > > to be false. > > > > > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" userspace > > > > > > > > > may > > > > > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > > > > > applications need to be modified in order to get the behaviour > > > > > > > recovered which is an impossible taks. > > > > > > > > > > > > > > Any idea on this? > > > > > > > > > > > > > > Thanks > > > > > > > > So, let's say we had a modparam? Enough for this customer? > > > > WDYT? > > > > > > Just to make sure I understand the proposal. > > > > > > Did you mean a module parameter like "inherit_owner_by_default"? I > > > think it would be fine if we make it false by default. > > > > > > Thanks > > > > I think we should keep it true by default, changing the default > > risks regressing what we already fixes. > > I think it's not a regression since it comes since the day vhost is > introduced. To my understanding the real regression is the user space > noticeable behaviour changes introduced by vhost thread. > > > The specific customer can > > flip the modparam and be happy. > > If you stick to the false as default, I'm fine. > > Thanks That would be yet another behaviour change. I think one was enough, don't you think? > > > > > > > > > > -- > > > > MST > > > > > >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
Thank you for the comments; I will prepare a new patch version. Thanks, Cindy On Wed, May 14, 2025 at 10:53 AM Jason Wang wrote: > > On Tue, May 13, 2025 at 3:09 PM Michael S. Tsirkin wrote: > > > > On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > > > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > > > wrote: > > > > > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > > > wrote: > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` > > > > > > > > > > ioctl. > > > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the > > > > > > > > > > ioctl > > > > > > > > > > is disabled, and any attempt to use it will result in > > > > > > > > > > failure. > > > > > > > > > > > > > > > > > > I think we need to describe why the default value was chosen > > > > > > > > > to be false. > > > > > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" userspace > > > > > > > > > may > > > > > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > > > > > applications need to be modified in order to get the behaviour > > > > > > > recovered which is an impossible taks. > > > > > > > > > > > > > > Any idea on this? > > > > > > > > > > > > > > Thanks > > > > > > > > So, let's say we had a modparam? Enough for this customer? > > > > WDYT? > > > > > > Just to make sure I understand the proposal. > > > > > > Did you mean a module parameter like "inherit_owner_by_default"? I > > > think it would be fine if we make it false by default. > > > > > > Thanks > > > > I think we should keep it true by default, changing the default > > risks regressing what we already fixes. > > I think it's not a regression since it comes since the day vhost is > introduced. To my understanding the real regression is the user space > noticeable behaviour changes introduced by vhost thread. > > > The specific customer can > > flip the modparam and be happy. > > If you stick to the false as default, I'm fine. > > Thanks > > > > > > > > > > > -- > > > > MST > > > > > > >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Tue, May 13, 2025 at 3:09 PM Michael S. Tsirkin wrote: > > On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin wrote: > > > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > > wrote: > > > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > > wrote: > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > > wrote: > > > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` > > > > > > > > > ioctl. > > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the > > > > > > > > > ioctl > > > > > > > > > is disabled, and any attempt to use it will result in failure. > > > > > > > > > > > > > > > > I think we need to describe why the default value was chosen to > > > > > > > > be false. > > > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" userspace > > > > > > > > may > > > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > > > > applications need to be modified in order to get the behaviour > > > > > > recovered which is an impossible taks. > > > > > > > > > > > > Any idea on this? > > > > > > > > > > > > Thanks > > > > > > So, let's say we had a modparam? Enough for this customer? > > > WDYT? > > > > Just to make sure I understand the proposal. > > > > Did you mean a module parameter like "inherit_owner_by_default"? I > > think it would be fine if we make it false by default. > > > > Thanks > > I think we should keep it true by default, changing the default > risks regressing what we already fixes. I think it's not a regression since it comes since the day vhost is introduced. To my understanding the real regression is the user space noticeable behaviour changes introduced by vhost thread. > The specific customer can > flip the modparam and be happy. If you stick to the false as default, I'm fine. Thanks > > > > > > > -- > > > MST > > > >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Tue, May 13, 2025 at 12:08:51PM +0800, Jason Wang wrote: > On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin wrote: > > > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang > > > > > wrote: > > > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > > wrote: > > > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote: > > > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` > > > > > > > > ioctl. > > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl > > > > > > > > is disabled, and any attempt to use it will result in failure. > > > > > > > > > > > > > > I think we need to describe why the default value was chosen to > > > > > > > be false. > > > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" userspace may > > > > > > > > > > > > I meant "true" actually. > > > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > > > applications need to be modified in order to get the behaviour > > > > > recovered which is an impossible taks. > > > > > > > > > > Any idea on this? > > > > > > > > > > Thanks > > > > So, let's say we had a modparam? Enough for this customer? > > WDYT? > > Just to make sure I understand the proposal. > > Did you mean a module parameter like "inherit_owner_by_default"? I > think it would be fine if we make it false by default. > > Thanks I think we should keep it true by default, changing the default risks regressing what we already fixes. The specific customer can flip the modparam and be happy. > > > > -- > > MST > >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Wed, Apr 30, 2025 at 5:27 PM Michael S. Tsirkin wrote: > > On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin wrote: > > > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang wrote: > > > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang > > > > > wrote: > > > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote: > > > > > > > > > > > > > > Introduce a new config knob > > > > > > > `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl. > > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl > > > > > > > is disabled, and any attempt to use it will result in failure. > > > > > > > > > > > > I think we need to describe why the default value was chosen to be > > > > > > false. > > > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > > > inherit_owner was set to false: this means "legacy" userspace may > > > > > > > > > > I meant "true" actually. > > > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > > applications need to be modified in order to get the behaviour > > > > recovered which is an impossible taks. > > > > > > > > Any idea on this? > > > > > > > > Thanks > > So, let's say we had a modparam? Enough for this customer? > WDYT? Just to make sure I understand the proposal. Did you mean a module parameter like "inherit_owner_by_default"? I think it would be fine if we make it false by default. Thanks > > -- > MST >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Wed, Apr 30, 2025 at 11:34:49AM +0800, Jason Wang wrote: > On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin wrote: > > > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang wrote: > > > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang wrote: > > > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote: > > > > > > > > > > > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl. > > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl > > > > > > is disabled, and any attempt to use it will result in failure. > > > > > > > > > > I think we need to describe why the default value was chosen to be > > > > > false. > > > > > > > > > > What's more, should we document the implications here? > > > > > > > > > > inherit_owner was set to false: this means "legacy" userspace may > > > > > > > > I meant "true" actually. > > > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > > applications need to be modified in order to get the behaviour > > > recovered which is an impossible taks. > > > > > > Any idea on this? > > > > > > Thanks So, let's say we had a modparam? Enough for this customer? WDYT? -- MST
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Tue, Apr 29, 2025 at 6:56 PM Michael S. Tsirkin wrote: > > On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang wrote: > > > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang wrote: > > > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote: > > > > > > > > > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl. > > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl > > > > > is disabled, and any attempt to use it will result in failure. > > > > > > > > I think we need to describe why the default value was chosen to be > > > > false. > > > > > > > > What's more, should we document the implications here? > > > > > > > > inherit_owner was set to false: this means "legacy" userspace may > > > > > > I meant "true" actually. > > > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > > applications need to be modified in order to get the behaviour > > recovered which is an impossible taks. > > > > Any idea on this? > > > > Thanks > > At this point, as we changed the behaviour, we have two types of legacy > applications > - ones expecting inherit_owner false > - ones expecting inherit_owner true Considering vhost has been used for more than a decade, I would expect more will expect inhert_owner to be false. > > Whatever we do, some of these will have to be changed. If we must choose one to break, I'd expect to break less. > Given current > kernel has it as true, and given it is a cleaner behaviour that will > keep working when we disable CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL in 10 > years, I think it's the better default. The problem is, if we set it to true and only an ioctl will bring the old behavior back. Who will use this ioctl? We can't modify all legacy applications. > If you want to change it transparently, look for ways to > distinguish between the two types. > > The application in question is qemu, is it not? Looks not, it's the application or management layer that tries to set the affinity to the owner of the vhost. For example, if I start a testpmd + vhost_net and pinn testpmd runs on cpu0. I will get half of the performance since vhost will contend with cpu with testpmd. > I do not see how sticking an ioctl call into its source is such > a big deal, if this is what we want to do. > A bit of short term pain but we get clear maintainable semantics. What's more important, the way that introduces new fork behaviors without a new uAPI is a bug. We need to fix that by introducing new uAPI for the behaviour. This seems to be the short term pain instead of introducing new uAPI for the old behaviour. Thanks > > -- > MST >
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Tue, Apr 29, 2025 at 11:39:37AM +0800, Jason Wang wrote: > On Mon, Apr 21, 2025 at 11:46 AM Jason Wang wrote: > > > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang wrote: > > > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote: > > > > > > > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl. > > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl > > > > is disabled, and any attempt to use it will result in failure. > > > > > > I think we need to describe why the default value was chosen to be false. > > > > > > What's more, should we document the implications here? > > > > > > inherit_owner was set to false: this means "legacy" userspace may > > > > I meant "true" actually. > > MIchael, I'd expect inherit_owner to be false. Otherwise legacy > applications need to be modified in order to get the behaviour > recovered which is an impossible taks. > > Any idea on this? > > Thanks At this point, as we changed the behaviour, we have two types of legacy applications - ones expecting inherit_owner false - ones expecting inherit_owner true Whatever we do, some of these will have to be changed. Given current kernel has it as true, and given it is a cleaner behaviour that will keep working when we disable CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL in 10 years, I think it's the better default. If you want to change it transparently, look for ways to distinguish between the two types. The application in question is qemu, is it not? I do not see how sticking an ioctl call into its source is such a big deal, if this is what we want to do. A bit of short term pain but we get clear maintainable semantics. -- MST
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Mon, Apr 21, 2025 at 11:46 AM Jason Wang wrote: > > On Mon, Apr 21, 2025 at 11:45 AM Jason Wang wrote: > > > > On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote: > > > > > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`, > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl. > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl > > > is disabled, and any attempt to use it will result in failure. > > > > I think we need to describe why the default value was chosen to be false. > > > > What's more, should we document the implications here? > > > > inherit_owner was set to false: this means "legacy" userspace may > > I meant "true" actually. MIchael, I'd expect inherit_owner to be false. Otherwise legacy applications need to be modified in order to get the behaviour recovered which is an impossible taks. Any idea on this? Thanks
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Tue, Apr 22, 2025 at 9:50 PM Stefano Garzarella wrote:
>
> On Mon, Apr 21, 2025 at 10:44:10AM +0800, Cindy Lu wrote:
> >Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> >to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> >When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> >is disabled, and any attempt to use it will result in failure.
> >
> >Signed-off-by: Cindy Lu
> >---
> > drivers/vhost/Kconfig | 15 +++
> > drivers/vhost/vhost.c | 3 +++
> > 2 files changed, 18 insertions(+)
> >
> >diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> >index 020d4fbb947c..bc8fadb06f98 100644
> >--- a/drivers/vhost/Kconfig
> >+++ b/drivers/vhost/Kconfig
> >@@ -96,3 +96,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > If unsure, say "N".
> >
> > endif
> >+
> >+config VHOST_ENABLE_FORK_OWNER_IOCTL
> >+ bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> >+ default n
> >+ help
> >+This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> >+userspace applications to modify the thread mode for vhost devices.
> >+
> >+ By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> >+ meaning the ioctl is disabled and any operation using this ioctl
> >+ will fail.
> >+ When the configuration is enabled (y), the ioctl becomes
> >+ available, allowing users to set the mode if needed.
>
> I think I already pointed out, but here there is a mix of tabs and
> spaces that IMHO we should fix.
>
Sorry, I missed this comment while preparing the patch; I’ll fix it.
> >+
> >+If unsure, say "N".
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index fb0c7fb43f78..568e43cb54a9 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int
> >ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> >+
> >+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
>
> As I mentioned in the previous version, IMO this patch should be merged
> with the previous patch. I don't think it is good for bisection to have
> a commit with an IOCTL supported in any case and in the next commit
> instead supported only through a config.
>
> Maybe I'm missing something, but what's the point of having a separate
> patch for this?
>
> Thanks,
> Stefano
>
will fix this
thanks
cindy
> > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > u8 inherit_owner;
> > /*inherit_owner can only be modified before owner is set*/
> >@@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int
> >ioctl, void __user *argp)
> > r = 0;
> > goto done;
> > }
> >+#endif
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> >--
> >2.45.0
> >
>
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Mon, Apr 21, 2025 at 10:44:10AM +0800, Cindy Lu wrote:
Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
is disabled, and any attempt to use it will result in failure.
Signed-off-by: Cindy Lu
---
drivers/vhost/Kconfig | 15 +++
drivers/vhost/vhost.c | 3 +++
2 files changed, 18 insertions(+)
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 020d4fbb947c..bc8fadb06f98 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -96,3 +96,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
If unsure, say "N".
endif
+
+config VHOST_ENABLE_FORK_OWNER_IOCTL
+ bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
+ default n
+ help
+ This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
+ userspace applications to modify the thread mode for vhost devices.
+
+ By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
+ meaning the ioctl is disabled and any operation using this ioctl
+ will fail.
+ When the configuration is enabled (y), the ioctl becomes
+ available, allowing users to set the mode if needed.
I think I already pointed out, but here there is a mix of tabs and
spaces that IMHO we should fix.
+
+ If unsure, say "N".
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index fb0c7fb43f78..568e43cb54a9 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int
ioctl, void __user *argp)
r = vhost_dev_set_owner(d);
goto done;
}
+
+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
As I mentioned in the previous version, IMO this patch should be merged
with the previous patch. I don't think it is good for bisection to have
a commit with an IOCTL supported in any case and in the next commit
instead supported only through a config.
Maybe I'm missing something, but what's the point of having a separate
patch for this?
Thanks,
Stefano
if (ioctl == VHOST_FORK_FROM_OWNER) {
u8 inherit_owner;
/*inherit_owner can only be modified before owner is set*/
@@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int
ioctl, void __user *argp)
r = 0;
goto done;
}
+#endif
/* You must be the owner to do anything else */
r = vhost_dev_check_owner(d);
if (r)
--
2.45.0
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Mon, Apr 21, 2025 at 11:45 AM Jason Wang wrote:
>
> On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote:
> >
> > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > is disabled, and any attempt to use it will result in failure.
>
> I think we need to describe why the default value was chosen to be false.
>
> What's more, should we document the implications here?
>
> inherit_owner was set to false: this means "legacy" userspace may
I meant "true" actually.
> still be broken unless it can do VHOST_FORK_FROM_OWNER which is
> impractical. Does this defeat the purpose of this series actually?
>
> >
> > Signed-off-by: Cindy Lu
> > ---
> > drivers/vhost/Kconfig | 15 +++
> > drivers/vhost/vhost.c | 3 +++
> > 2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 020d4fbb947c..bc8fadb06f98 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -96,3 +96,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > If unsure, say "N".
> >
> > endif
> > +
> > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > + default n
> > + help
> > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > + userspace applications to modify the thread mode for vhost
> > devices.
> > +
> > + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > + meaning the ioctl is disabled and any operation using this ioctl
> > + will fail.
> > + When the configuration is enabled (y), the ioctl becomes
> > + available, allowing users to set the mode if needed.
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index fb0c7fb43f78..568e43cb54a9 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned
> > int ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> > +
> > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > u8 inherit_owner;
> > /*inherit_owner can only be modified before owner is set*/
> > @@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned
> > int ioctl, void __user *argp)
> > r = 0;
> > goto done;
> > }
> > +#endif
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> > --
> > 2.45.0
> >
>
> Thanks
Thanks
Re: [PATCH v9 4/4] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
On Mon, Apr 21, 2025 at 10:45 AM Cindy Lu wrote:
>
> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> is disabled, and any attempt to use it will result in failure.
I think we need to describe why the default value was chosen to be false.
What's more, should we document the implications here?
inherit_owner was set to false: this means "legacy" userspace may
still be broken unless it can do VHOST_FORK_FROM_OWNER which is
impractical. Does this defeat the purpose of this series actually?
>
> Signed-off-by: Cindy Lu
> ---
> drivers/vhost/Kconfig | 15 +++
> drivers/vhost/vhost.c | 3 +++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 020d4fbb947c..bc8fadb06f98 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -96,3 +96,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> If unsure, say "N".
>
> endif
> +
> +config VHOST_ENABLE_FORK_OWNER_IOCTL
> + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> + default n
> + help
> + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> + userspace applications to modify the thread mode for vhost devices.
> +
> + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> + meaning the ioctl is disabled and any operation using this ioctl
> + will fail.
> + When the configuration is enabled (y), the ioctl becomes
> + available, allowing users to set the mode if needed.
> +
> + If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fb0c7fb43f78..568e43cb54a9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int
> ioctl, void __user *argp)
> r = vhost_dev_set_owner(d);
> goto done;
> }
> +
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> if (ioctl == VHOST_FORK_FROM_OWNER) {
> u8 inherit_owner;
> /*inherit_owner can only be modified before owner is set*/
> @@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int
> ioctl, void __user *argp)
> r = 0;
> goto done;
> }
> +#endif
> /* You must be the owner to do anything else */
> r = vhost_dev_check_owner(d);
> if (r)
> --
> 2.45.0
>
Thanks
