Re: [libvirt PATCH v6 00/30] Add support for persistent mediated devices
On Wed, 7 Apr 2021 08:05:17 +0200 Erik Skultety wrote: > On Thu, Apr 01, 2021 at 10:18:45AM -0500, Jonathon Jongsma wrote: > > On Wed, 31 Mar 2021 16:00:48 +0200 > > Erik Skultety wrote: > > > > > On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote: > > > > > > > This patch series follows the previously-merged series which > > > > added support for transient mediated devices. This series > > > > expands mdev support to include persistent device definitions. > > > > Again, it relies on mdevctl as the backend. > > > > > > > > It follows the common libvirt pattern of APIs by adding the > > > > following new APIs for node devices: > > > > - virNodeDeviceDefineXML() - defines a persistent device > > > > - virNodeDeviceUndefine() - undefines a persistent device > > > > - virNodeDeviceCreate() - starts a previously-defined device > > > > > > > > It also adds virsh commands mapping to these new APIs: > > > > nodedev-define, nodedev-undefine, and nodedev-start. > > > > > > > > Since we rely on mdevctl for the definition of mediated > > > > devices, we need a way to stay up-to-date with devices that are > > > > defined by mdevctl (outside of libvirt). The method for > > > > staying up-to-date is currently a little bit crude due to the > > > > fact that mdevctl does not emit any events when new devices are > > > > added or removed. As a workaround, we create a file monitor for > > > > the mdevctl config directory and re-query mdevctl when we > > > > detect changes within that directory. In the future, mdevctl > > > > may introduce a more elegant solution. > > > > > > > > Changes in v6: > > > > - rebase to git master again > > > > - remove typedefs for various *Ptr types, since they're now > > > > discouraged in libvirt. > > > > > > Okay, so I think I ACKed all of the patches now (if not, let me > > > know which one I missed). I tested the patches, found 3 leaks, > > > one of them I mentioned here, 2 were related to the code but not > > > a direct result of this series I believe, so I'll tackle them > > > some other time as a follow up. Overall, I think we're good to > > > push this starting with the 7.3.0 cycle. > > > > I think everything else has been acked. > > > > > > > > Now, in v4 I promised to share my adjustments I made on top of > > > your code. Some of them you already handled yourself in v6, so I > > > rebased and performed a bunch of changes, so here they are: > > > https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments > > > > > > Note, that I only split them into multiple patches so that they're > > > easier to read, but I'm very well aware that probably none of them > > > cannot be compiled on its own (I didn't bother to be that > > > thorough), it's just to give you an idea what I've failed to > > > express with words until we came to this v6. Please let me know > > > your opinion on the changes so that: a) I can send it as a > > > *proper* follow up series b) you can incorporate some of the > > > changes into your series > > > > Thanks for that. I think the changes look reasonable, and I think > > the benefits outweigh the drawbacks. I have a few changes I'd like > > to make to your commits. See the top 3 commits at [1] for details. > > Would you like me to just incorporate your stuff into my original > > series, or keep them as separate commits on top of my series? > > I like your ^additional cleanup. > > As for the 30 patches - I'd like to avoid having to go through most > of them again, so no, please go ahead an merge this series as is. The > follow up changes that started to discuss are not functional anyway, > so posting them separately is actually preferable. > > As for the follow up series - now that you see what my intentions wrt > abstracting the mdevctl cmdline building code as much as possible > were, feel free to take my commits, apply your 3 changes, shuffle > them around as you need, and post it as a proper follow up series > that can compile after every single patch :) and I'll review it. Sounds good. Will send a followup series shortly. Thanks! Jonathon
Re: [libvirt PATCH v6 00/30] Add support for persistent mediated devices
On Thu, Apr 01, 2021 at 10:18:45AM -0500, Jonathon Jongsma wrote: > On Wed, 31 Mar 2021 16:00:48 +0200 > Erik Skultety wrote: > > > On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote: > > > This patch series follows the previously-merged series which added > > > support for transient mediated devices. This series expands mdev > > > support to include persistent device definitions. Again, it relies > > > on mdevctl as the backend. > > > > > > It follows the common libvirt pattern of APIs by adding the > > > following new APIs for node devices: > > > - virNodeDeviceDefineXML() - defines a persistent device > > > - virNodeDeviceUndefine() - undefines a persistent device > > > - virNodeDeviceCreate() - starts a previously-defined device > > > > > > It also adds virsh commands mapping to these new APIs: > > > nodedev-define, nodedev-undefine, and nodedev-start. > > > > > > Since we rely on mdevctl for the definition of mediated devices, we > > > need a way to stay up-to-date with devices that are defined by > > > mdevctl (outside of libvirt). The method for staying up-to-date is > > > currently a little bit crude due to the fact that mdevctl does not > > > emit any events when new devices are added or removed. As a > > > workaround, we create a file monitor for the mdevctl config > > > directory and re-query mdevctl when we detect changes within that > > > directory. In the future, mdevctl may introduce a more elegant > > > solution. > > > > > > Changes in v6: > > > - rebase to git master again > > > - remove typedefs for various *Ptr types, since they're now > > > discouraged in libvirt. > > > > Okay, so I think I ACKed all of the patches now (if not, let me know > > which one I missed). I tested the patches, found 3 leaks, one of them > > I mentioned here, 2 were related to the code but not a direct result > > of this series I believe, so I'll tackle them some other time as a > > follow up. Overall, I think we're good to push this starting with the > > 7.3.0 cycle. > > I think everything else has been acked. > > > > > Now, in v4 I promised to share my adjustments I made on top of your > > code. Some of them you already handled yourself in v6, so I rebased > > and performed a bunch of changes, so here they are: > > https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments > > > > Note, that I only split them into multiple patches so that they're > > easier to read, but I'm very well aware that probably none of them > > cannot be compiled on its own (I didn't bother to be that thorough), > > it's just to give you an idea what I've failed to express with words > > until we came to this v6. Please let me know your opinion on the > > changes so that: a) I can send it as a *proper* follow up series > > b) you can incorporate some of the changes into your series > > > > Thanks for that. I think the changes look reasonable, and I think the > benefits outweigh the drawbacks. I have a few changes I'd like to make > to your commits. See the top 3 commits at [1] for details. Would you > like me to just incorporate your stuff into my original series, or keep > them as separate commits on top of my series? I like your ^additional cleanup. As for the 30 patches - I'd like to avoid having to go through most of them again, so no, please go ahead an merge this series as is. The follow up changes that started to discuss are not functional anyway, so posting them separately is actually preferable. As for the follow up series - now that you see what my intentions wrt abstracting the mdevctl cmdline building code as much as possible were, feel free to take my commits, apply your 3 changes, shuffle them around as you need, and post it as a proper follow up series that can compile after every single patch :) and I'll review it. Regards, Erik
Re: [libvirt PATCH v6 00/30] Add support for persistent mediated devices
On Wed, 31 Mar 2021 16:00:48 +0200 Erik Skultety wrote: > On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote: > > This patch series follows the previously-merged series which added > > support for transient mediated devices. This series expands mdev > > support to include persistent device definitions. Again, it relies > > on mdevctl as the backend. > > > > It follows the common libvirt pattern of APIs by adding the > > following new APIs for node devices: > > - virNodeDeviceDefineXML() - defines a persistent device > > - virNodeDeviceUndefine() - undefines a persistent device > > - virNodeDeviceCreate() - starts a previously-defined device > > > > It also adds virsh commands mapping to these new APIs: > > nodedev-define, nodedev-undefine, and nodedev-start. > > > > Since we rely on mdevctl for the definition of mediated devices, we > > need a way to stay up-to-date with devices that are defined by > > mdevctl (outside of libvirt). The method for staying up-to-date is > > currently a little bit crude due to the fact that mdevctl does not > > emit any events when new devices are added or removed. As a > > workaround, we create a file monitor for the mdevctl config > > directory and re-query mdevctl when we detect changes within that > > directory. In the future, mdevctl may introduce a more elegant > > solution. > > > > Changes in v6: > > - rebase to git master again > > - remove typedefs for various *Ptr types, since they're now > > discouraged in libvirt. > > Okay, so I think I ACKed all of the patches now (if not, let me know > which one I missed). I tested the patches, found 3 leaks, one of them > I mentioned here, 2 were related to the code but not a direct result > of this series I believe, so I'll tackle them some other time as a > follow up. Overall, I think we're good to push this starting with the > 7.3.0 cycle. I think everything else has been acked. > > Now, in v4 I promised to share my adjustments I made on top of your > code. Some of them you already handled yourself in v6, so I rebased > and performed a bunch of changes, so here they are: > https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments > > Note, that I only split them into multiple patches so that they're > easier to read, but I'm very well aware that probably none of them > cannot be compiled on its own (I didn't bother to be that thorough), > it's just to give you an idea what I've failed to express with words > until we came to this v6. Please let me know your opinion on the > changes so that: a) I can send it as a *proper* follow up series > b) you can incorporate some of the changes into your series > Thanks for that. I think the changes look reasonable, and I think the benefits outweigh the drawbacks. I have a few changes I'd like to make to your commits. See the top 3 commits at [1] for details. Would you like me to just incorporate your stuff into my original series, or keep them as separate commits on top of my series? Jonathon [1] https://gitlab.com/jjongsma/libvirt/-/commits/mdevctl-adjustments/
Re: [libvirt PATCH v6 00/30] Add support for persistent mediated devices
On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote: > This patch series follows the previously-merged series which added support for > transient mediated devices. This series expands mdev support to include > persistent device definitions. Again, it relies on mdevctl as the backend. > > It follows the common libvirt pattern of APIs by adding the following new APIs > for node devices: > - virNodeDeviceDefineXML() - defines a persistent device > - virNodeDeviceUndefine() - undefines a persistent device > - virNodeDeviceCreate() - starts a previously-defined device > > It also adds virsh commands mapping to these new APIs: nodedev-define, > nodedev-undefine, and nodedev-start. > > Since we rely on mdevctl for the definition of mediated devices, we need a way > to stay up-to-date with devices that are defined by mdevctl (outside of > libvirt). The method for staying up-to-date is currently a little bit crude > due to the fact that mdevctl does not emit any events when new devices are > added or removed. As a workaround, we create a file monitor for the mdevctl > config directory and re-query mdevctl when we detect changes within that > directory. In the future, mdevctl may introduce a more elegant solution. > > Changes in v6: > - rebase to git master again > - remove typedefs for various *Ptr types, since they're now discouraged in >libvirt. Okay, so I think I ACKed all of the patches now (if not, let me know which one I missed). I tested the patches, found 3 leaks, one of them I mentioned here, 2 were related to the code but not a direct result of this series I believe, so I'll tackle them some other time as a follow up. Overall, I think we're good to push this starting with the 7.3.0 cycle. Now, in v4 I promised to share my adjustments I made on top of your code. Some of them you already handled yourself in v6, so I rebased and performed a bunch of changes, so here they are: https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments Note, that I only split them into multiple patches so that they're easier to read, but I'm very well aware that probably none of them cannot be compiled on its own (I didn't bother to be that thorough), it's just to give you an idea what I've failed to express with words until we came to this v6. Please let me know your opinion on the changes so that: a) I can send it as a *proper* follow up series b) you can incorporate some of the changes into your series I don't care either way. Regards and thanks for being patient with the review process :), Erik
