| Message ID | 20201209205413.3391139-1-vipinsh@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cgroup: KVM: New Encryption IDs cgroup controller | expand |
Hello, Rough take after skimming: * I don't have an overall objection. In terms of behavior, the only thing which stood out was input rejection depending on the current usage. The preferred way of handling that is rejecting future allocations rather than failing configuration as that makes it impossible e.g. to lower limit and drain existing usages from outside the container. * However, the boilerplate to usefulness ratio doesn't look too good and I wonder whether what we should do is adding a generic "misc" controller which can host this sort of static hierarchical counting. I'll think more on it. Thanks.
On 09.12.20 21:58, Tejun Heo wrote: > Hello, > > Rough take after skimming: > > * I don't have an overall objection. In terms of behavior, the only thing > which stood out was input rejection depending on the current usage. The > preferred way of handling that is rejecting future allocations rather than > failing configuration as that makes it impossible e.g. to lower limit and > drain existing usages from outside the container. > > * However, the boilerplate to usefulness ratio doesn't look too good and I > wonder whether what we should do is adding a generic "misc" controller > which can host this sort of static hierarchical counting. I'll think more > on it. We first dicussed to have encryption_ids.stat encryption_ids.max encryption_ids.current and we added the sev in later, so that we can also have tdx, seid, sgx or whatever. Maybe also 2 or more things at the same time. Right now this code has encryption_ids.sev.stat encryption_ids.sev.max encryption_ids.sev.current And it would be trivial to extend it to have encryption_ids.seid.stat encryption_ids.seid.max encryption_ids.seid.current on s390 instead (for our secure guests). So in the end this is almost already a misc controller, the only thing that we need to change is the capability to also define things other than encryption.*.* And of course we would need to avoid adding lots of random garbage to such a thing. But if you feel ok with the burden to keep things kind of organized a misc controller would certainly work for the encryption ID usecase as well. So I would be fine with the thing as is or a misc controlĺer. Christian
On Thu, 10 Dec 2020, Christian Borntraeger wrote: > > * However, the boilerplate to usefulness ratio doesn't look too good and I > > wonder whether what we should do is adding a generic "misc" controller > > which can host this sort of static hierarchical counting. I'll think more > > on it. > > We first dicussed to have > encryption_ids.stat > encryption_ids.max > encryption_ids.current > > and we added the sev in later, so that we can also have tdx, seid, sgx or whatever. > Maybe also 2 or more things at the same time. > > Right now this code has > > encryption_ids.sev.stat > encryption_ids.sev.max > encryption_ids.sev.current > > And it would be trivial to extend it to have > encryption_ids.seid.stat > encryption_ids.seid.max > encryption_ids.seid.current > on s390 instead (for our secure guests). > > So in the end this is almost already a misc controller, the only thing that we > need to change is the capability to also define things other than encryption.*.* > And of course we would need to avoid adding lots of random garbage to such a thing. > > But if you feel ok with the burden to keep things kind of organized a misc > controller would certainly work for the encryption ID usecase as well. > So I would be fine with the thing as is or a misc controlĺer. > Yeah, I think generalization of this would come in the form of either (1) the dumping ground of an actual "misc" controller, that you elude to, or (2) a kernel abstraction so you can spin up your own generic controller that has the {current, max, stat} support. In the case of the latter, encryption IDs becomes a user of that abstraction. Concern with a single misc controller would be that any subsystem that wants to use it has to exactly fit this support: current, max, stat, nothing more. The moment a controller needs some additional support, and its controller is already implemented in previous kernel versionv as a part of "misc," we face a problem. On the other hand, a kernel abstraction that provides just the basic {current, max, stat} support might be interesting if it can be extended by the subsystem instance using it.
Hello, On Thu, Dec 10, 2020 at 03:44:35PM -0800, David Rientjes wrote: > Concern with a single misc controller would be that any subsystem that > wants to use it has to exactly fit this support: current, max, stat, > nothing more. The moment a controller needs some additional support, and > its controller is already implemented in previous kernel versionv as a > part of "misc," we face a problem. Yeah, that's a valid concern, but given the history, there doesn't seem to be much need beyond that for these use cases and the limited need seems inherent to the way the resources are defined and consumed. So yeah, it can either way. Thanks.
On Wed, Dec 9, 2020 at 12:59 PM Tejun Heo <tj@kernel.org> wrote: > * I don't have an overall objection. In terms of behavior, the only thing > which stood out was input rejection depending on the current usage. The > preferred way of handling that is rejecting future allocations rather than > failing configuration as that makes it impossible e.g. to lower limit and > drain existing usages from outside the container. Thanks. In next version of the patch I will remove rejection of max value based on current usage. On Wed, Dec 16, 2020 at 7:27 AM Tejun Heo <tj@kernel.org> wrote: > On Thu, Dec 10, 2020 at 03:44:35PM -0800, David Rientjes wrote: > > Concern with a single misc controller would be that any subsystem that > > wants to use it has to exactly fit this support: current, max, stat, > > nothing more. The moment a controller needs some additional support, and > > its controller is already implemented in previous kernel versionv as a > > part of "misc," we face a problem. > > Yeah, that's a valid concern, but given the history, there doesn't seem to > be much need beyond that for these use cases and the limited need seems > inherent to the way the resources are defined and consumed. So yeah, it can > either way. I think a misc controller should be able support other "Resource Distribution Models" mentioned in the cgroup v2 documentation besides limits. There might be use cases in future which want to use weight, protection or allocation models. If that happens it will be more difficult to support these different resources. This will also mean the same hierarchy might get charged differently by the same controller. I like the idea of having a separate controller to keep the code simple and easier for maintenance. If you decide to have a separate misc controller please let me know what will be the overall expectations and I can change my patch to reflect that, otherwise I can send out a new patch with just removal of max input rejection. My understanding with a "misc" controller is it will be something like - cgroup v2 cgroup/misc.encryption_ids.{sev, tdx, seid}.{stat, max, current} - cgroup v1 cgroup/misc/misc.encryption_ids.{sev, tdx, seid}.{stat, max, current} Thanks Vipin
Happy new year! On Wed, Dec 16, 2020 at 12:02:37PM -0800, Vipin Sharma wrote: > I like the idea of having a separate controller to keep the code simple and > easier for maintenance. Yeah, the more I think about it, keeping it separate seems like the right thing to do. What bothers me primarily is that the internal logic is identical between the RDMA controller and this one. If you wanna try factoring them out into library, great. If not, I don't think it should block merging this controller. We can get to refactoring later. Thanks.
On Tue, Jan 05, 2021 at 10:36:40AM -0500, Tejun Heo wrote: > Happy new year! > > On Wed, Dec 16, 2020 at 12:02:37PM -0800, Vipin Sharma wrote: > > I like the idea of having a separate controller to keep the code simple and > > easier for maintenance. > > Yeah, the more I think about it, keeping it separate seems like the right > thing to do. What bothers me primarily is that the internal logic is > identical between the RDMA controller and this one. If you wanna try > factoring them out into library, great. If not, I don't think it should > block merging this controller. We can get to refactoring later. > Happy new year! Sounds great, I will send out a new patch which will not reject the new max limit based on the current usage. It will not include refactoring out common code between RDMA and Encryption ID controller. We can pursue that later. Thanks Vipin