| Message ID | 1596507196-27417-2-git-send-email-muneendra.kumar@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Application specific identification support | expand |
Hi, [cc Tejun] On Tue, Aug 04, 2020 at 07:43:01AM +0530, Muneendra wrote: > This Patch added a unique application identifier i.e > blkio.app_identifier knob to blkio controller which > allows identification of traffic sources at an > individual cgroup based Applications > (ex:virtual machine (VM))level in both host and > fabric infrastructure. Is there any specific reason the commit message is formatted in this way? It looks a bit strange not using a bit more of horizontal space. > Also provided an interface blkcg_get_app_identifier to > grab the app identifier associated with a bio. > > Added a sysfs interface blkio.app_identifier to get/set the appid. > > This capability can be utilized by multiple block transport infrastructure > like fc,iscsi,roce .. > > Signed-off-by: Muneendra <muneendra.kumar@broadcom.com> > --- > block/blk-cgroup.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/blk-cgroup.h | 19 +++++++++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > index 0ecc897b225c..697eccb3ba7a 100644 > --- a/block/blk-cgroup.c > +++ b/block/blk-cgroup.c > @@ -492,6 +492,33 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css, > return 0; > } > > +static int blkcg_read_appid(struct seq_file *sf, void *v) > +{ > + struct blkcg *blkcg = css_to_blkcg(seq_css(sf)); > + > + seq_printf(sf, "%s\n", blkcg->app_identifier); > + return 0; > +} > + > +static ssize_t blkcg_write_appid(struct kernfs_open_file *of, > + char *buf, size_t nbytes, loff_t off) > +{ > + struct cgroup_subsys_state *css = of_css(of); > + struct blkcg *blkcg = css_to_blkcg(css); > + struct blkcg_gq *blkg; > + int i; > + > + buf = strstrip(buf); > + if (blkcg) { > + if (nbytes < APPID_LEN) > + strlcpy(blkcg->app_identifier, buf, nbytes); strstrip() shortens the string but you still copy nbytes. > + else > + return -EINVAL; > + } > + return nbytes; > +} > + > + > const char *blkg_dev_name(struct blkcg_gq *blkg) > { > /* some drivers (floppy) instantiate a queue w/o disk registered */ > @@ -844,6 +871,11 @@ static struct cftype blkcg_legacy_files[] = { > .name = "reset_stats", > .write_u64 = blkcg_reset_stats, > }, > + { > + .name = "app_identifier", > + .write = blkcg_write_appid, > + .seq_show = blkcg_read_appid, > + }, I am no expert with cgroups. Isn't this just adding it to cgroup v1 only? > { } /* terminate */ > }; > > diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h > index a57ebe2f00ab..3676d7ebb19f 100644 > --- a/include/linux/blk-cgroup.h > +++ b/include/linux/blk-cgroup.h > @@ -30,6 +30,7 @@ > > /* Max limits for throttle policy */ > #define THROTL_IOPS_MAX UINT_MAX > +#define APPID_LEN 128 > > #ifdef CONFIG_BLK_CGROUP > > @@ -55,6 +56,7 @@ struct blkcg { > struct blkcg_policy_data *cpd[BLKCG_MAX_POLS]; > > struct list_head all_blkcgs_node; > + char app_identifier[APPID_LEN]; > #ifdef CONFIG_CGROUP_WRITEBACK > struct list_head cgwb_list; > #endif > @@ -239,6 +241,23 @@ static inline struct blkcg *css_to_blkcg(struct cgroup_subsys_state *css) > return css ? container_of(css, struct blkcg, css) : NULL; > } > > +/** > + * blkcg_get_app_identifier - grab the app identifier associated with a bio > + * @bio: target bio > + * > + * This returns the app identifier associated with a bio, > + * %NULL if not associated. > + * Callers are expected to either handle %NULL or know association has been > + * done prior to calling this. > + */ > +static inline char *blkcg_get_app_identifier(struct bio *bio) > +{ > + if (bio && (bio->bi_blkg) && > + (strlen(bio->bi_blkg->blkcg->app_identifier))) > + return bio->bi_blkg->blkcg->app_identifier; Too many brackets Thanks, Daniel
Hello, On Tue, Aug 04, 2020 at 01:31:30PM +0200, Daniel Wagner wrote: > Hi, > > [cc Tejun] > > On Tue, Aug 04, 2020 at 07:43:01AM +0530, Muneendra wrote: > > This Patch added a unique application identifier i.e > > blkio.app_identifier knob to blkio controller which > > allows identification of traffic sources at an > > individual cgroup based Applications > > (ex:virtual machine (VM))level in both host and > > fabric infrastructure. I'm not sure it makes sense to introduce custom IDs for these given that there already are unique per-host cgroup IDs which aren't recycled. Thanks.
On 8/4/2020 7:21 AM, Tejun Heo wrote: > Hello, > > On Tue, Aug 04, 2020 at 01:31:30PM +0200, Daniel Wagner wrote: >> Hi, >> >> [cc Tejun] >> >> On Tue, Aug 04, 2020 at 07:43:01AM +0530, Muneendra wrote: >>> This Patch added a unique application identifier i.e >>> blkio.app_identifier knob to blkio controller which >>> allows identification of traffic sources at an >>> individual cgroup based Applications >>> (ex:virtual machine (VM))level in both host and >>> fabric infrastructure. > I'm not sure it makes sense to introduce custom IDs for these given that > there already are unique per-host cgroup IDs which aren't recycled. > > Thanks. > If the VM moves to a different host, does the per-host cgroup IDs migrate with the VM ? If not, we need to have an identifier that moves with the VM and which is independent of what host the VM is residing on. -- james
On Wed, Aug 5, 2020 at 8:42 AM James Smart <james.smart@broadcom.com> wrote: > > On 8/4/2020 7:21 AM, Tejun Heo wrote: > > Hello, > > > > On Tue, Aug 04, 2020 at 01:31:30PM +0200, Daniel Wagner wrote: > >> Hi, > >> > >> [cc Tejun] > >> > >> On Tue, Aug 04, 2020 at 07:43:01AM +0530, Muneendra wrote: > >>> This Patch added a unique application identifier i.e > >>> blkio.app_identifier knob to blkio controller which > >>> allows identification of traffic sources at an > >>> individual cgroup based Applications > >>> (ex:virtual machine (VM))level in both host and > >>> fabric infrastructure. > > I'm not sure it makes sense to introduce custom IDs for these given that > > there already are unique per-host cgroup IDs which aren't recycled. > > > > Thanks. > > > > If the VM moves to a different host, does the per-host cgroup IDs > migrate with the VM ? If not, we need to have an identifier that moves > with the VM and which is independent of what host the VM is residing on. Hello James, Is it possible to reuse VM's mac address or other unique ID for such purpose? Thanks, Ming Lei
On 8/5/20 5:59 AM, Ming Lei wrote: > On Wed, Aug 5, 2020 at 8:42 AM James Smart <james.smart@broadcom.com> wrote: >> >> On 8/4/2020 7:21 AM, Tejun Heo wrote: >>> Hello, >>> >>> On Tue, Aug 04, 2020 at 01:31:30PM +0200, Daniel Wagner wrote: >>>> Hi, >>>> >>>> [cc Tejun] >>>> >>>> On Tue, Aug 04, 2020 at 07:43:01AM +0530, Muneendra wrote: >>>>> This Patch added a unique application identifier i.e >>>>> blkio.app_identifier knob to blkio controller which >>>>> allows identification of traffic sources at an >>>>> individual cgroup based Applications >>>>> (ex:virtual machine (VM))level in both host and >>>>> fabric infrastructure. >>> I'm not sure it makes sense to introduce custom IDs for these given that >>> there already are unique per-host cgroup IDs which aren't recycled. >>> >>> Thanks. >>> >> >> If the VM moves to a different host, does the per-host cgroup IDs >> migrate with the VM ? If not, we need to have an identifier that moves >> with the VM and which is independent of what host the VM is residing on. > > Hello James, > > Is it possible to reuse VM's mac address or other unique ID for such purpose? > None of these are guaranteed to be either present or unique. It's not uncommon to have VMs with more than one MAC address, and any other unique identifier like system UUID is not required to be present. Cheers, Hannes
Hello, On Wed, Aug 05, 2020 at 08:33:19AM +0200, Hannes Reinecke wrote: > None of these are guaranteed to be either present or unique. > It's not uncommon to have VMs with more than one MAC address, and any other > unique identifier like system UUID is not required to be present. I have a really hard time seeing how this fits into block cgroup interface. The last time we did something similar (tagging from cgroup interface side) was for netcls and it ended poorly. There are basic problems with e.g. delegation as these things are at odds with everything else sharing the interface. My not-too-well informed suggestions are either building mapping somewhere in the stack or at least implement the interface where it fits better so that people who don't need app_identifier don't see them and preferably can disable them. I don't mind cgroup data structs carrying extra bits for stuff which want to make use of the cgroup tree but I'm a pretty firm nack on the proposed interface. Thanks.
Hi Tejun, Our main requirement is to track the bio requests coming from different VM /container applications at the blk device layer(fc,scsi,nvme). By the time IO request comes to the blk device layer, the context of the application is lost and we can't track whose IO this belongs. In our approach we used the block cgroup to achieve this requirement. Since Requests also have access to the block cgroup via bio->bi_blkg->blkcg, and from there we can get the VM UUID. Therefore we added the VM UUID(app_identifier) to struct blkcg and define the accessors in blkcg_files and blkcg_legacy_files. Could you please let me know is there any another way where we can get the VM UUID info with the help of blkcg. Regards, Muneendra. -----Original Message----- From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo Sent: Wednesday, August 5, 2020 8:09 PM To: Hannes Reinecke <hare@suse.de> Cc: Ming Lei <tom.leiming@gmail.com>; James Smart <james.smart@broadcom.com>; Daniel Wagner <dwagner@suse.de>; Muneendra <muneendra.kumar@broadcom.com>; linux-block <linux-block@vger.kernel.org>; Linux SCSI List <linux-scsi@vger.kernel.org>; Paolo Bonzini <pbonzini@redhat.com>; Ewan Milne <emilne@redhat.com>; mkumar@redhat.com Subject: Re: [RFC 01/16] blkcg:Introduce blkio.app_identifier knob to blkio controller Hello, On Wed, Aug 05, 2020 at 08:33:19AM +0200, Hannes Reinecke wrote: > None of these are guaranteed to be either present or unique. > It's not uncommon to have VMs with more than one MAC address, and any > other unique identifier like system UUID is not required to be present. I have a really hard time seeing how this fits into block cgroup interface. The last time we did something similar (tagging from cgroup interface side) was for netcls and it ended poorly. There are basic problems with e.g. delegation as these things are at odds with everything else sharing the interface. My not-too-well informed suggestions are either building mapping somewhere in the stack or at least implement the interface where it fits better so that people who don't need app_identifier don't see them and preferably can disable them. I don't mind cgroup data structs carrying extra bits for stuff which want to make use of the cgroup tree but I'm a pretty firm nack on the proposed interface. Thanks. -- tejun
Hello, On Wed, Aug 05, 2020 at 10:44:56PM +0530, Muneendra Kumar M wrote: > Our main requirement is to track the bio requests coming from different VM > /container applications at the blk device layer(fc,scsi,nvme). > By the time IO request comes to the blk device layer, the context of the > application is lost and we can't track whose IO this belongs. I see. > In our approach we used the block cgroup to achieve this requirement. > Since Requests also have access to the block cgroup via > bio->bi_blkg->blkcg, and from there we can get the VM UUID. > Therefore we added the VM UUID(app_identifier) to struct blkcg and define > the accessors in blkcg_files and blkcg_legacy_files. > > Could you please let me know is there any another way where we can get the > VM UUID info with the help of blkcg. Using blkcg for identification does make sense given that that's the finest granularity which also covers buffered writes. However, the proposed is akin to adding per-thread application ID to procfs if it were per-thread instead of per-cgroup, which wouldn't fly well either given the specialized and (at least for now) niche nature of the feature. So, my vague suggestion is putting the interface so that it fits the usage scope and is close to whatever ultimate feature you wanna expose. I don't think I'm the right person to make concrete recommendations here given the lack of detailed context. Thanks.
On Thu, Aug 6, 2020 at 1:15 AM Muneendra Kumar M <muneendra.kumar@broadcom.com> wrote: > > Hi Tejun, > Our main requirement is to track the bio requests coming from different VM > /container applications at the blk device layer(fc,scsi,nvme). > By the time IO request comes to the blk device layer, the context of the > application is lost and we can't track whose IO this belongs. > > In our approach we used the block cgroup to achieve this requirement. > Since Requests also have access to the block cgroup via > bio->bi_blkg->blkcg, and from there we can get the VM UUID. > Therefore we added the VM UUID(app_identifier) to struct blkcg and define > the accessors in blkcg_files and blkcg_legacy_files. > > Could you please let me know is there any another way where we can get the > VM UUID info with the help of blkcg. As Tejun suggested, the mapping between bio->bi_blkg->blkcg and the unique ID could be built in usage scope, such as fabric infrastructure, something like xarray/hash may help to do that without much difficulty. Thanks, Ming
Hi Ming Lei, Thanks for the input. We will consider the points which you and Tejun has suggested . Regards, Muneendra. -----Original Message----- From: Ming Lei [mailto:tom.leiming@gmail.com] Sent: Thursday, August 6, 2020 7:52 AM To: Muneendra Kumar M <muneendra.kumar@broadcom.com> Cc: Tejun Heo <tj@kernel.org>; Hannes Reinecke <hare@suse.de>; James Smart <james.smart@broadcom.com>; Daniel Wagner <dwagner@suse.de>; linux-block <linux-block@vger.kernel.org>; Linux SCSI List <linux-scsi@vger.kernel.org>; Paolo Bonzini <pbonzini@redhat.com>; Ewan Milne <emilne@redhat.com>; mkumar@redhat.com Subject: Re: [RFC 01/16] blkcg:Introduce blkio.app_identifier knob to blkio controller On Thu, Aug 6, 2020 at 1:15 AM Muneendra Kumar M <muneendra.kumar@broadcom.com> wrote: > > Hi Tejun, > Our main requirement is to track the bio requests coming from > different VM /container applications at the blk device > layer(fc,scsi,nvme). > By the time IO request comes to the blk device layer, the context of > the application is lost and we can't track whose IO this belongs. > > In our approach we used the block cgroup to achieve this requirement. > Since Requests also have access to the block cgroup via > bio->bi_blkg->blkcg, and from there we can get the VM UUID. > Therefore we added the VM UUID(app_identifier) to struct blkcg and > define the accessors in blkcg_files and blkcg_legacy_files. > > Could you please let me know is there any another way where we can get > the VM UUID info with the help of blkcg. As Tejun suggested, the mapping between bio->bi_blkg->blkcg and the unique ID could be built in usage scope, such as fabric infrastructure, something like xarray/hash may help to do that without much difficulty. Thanks, Ming
On 06/08/20 04:22, Ming Lei wrote: >> Could you please let me know is there any another way where we can get the >> VM UUID info with the help of blkcg. > As Tejun suggested, the mapping between bio->bi_blkg->blkcg and the > unique ID could be built in usage scope, such as fabric > infrastructure, something like > xarray/hash may help to do that without much difficulty. So do you suggest adding one (or actually many) driver-specific files in /sys/bus/pci/drivers/lpfc or something like that? That seems very much inferior to just sticking it into blkcg. Even though this is only implemented by lpfc, other FC drivers will follow suit. Paolo
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 0ecc897b225c..697eccb3ba7a 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -492,6 +492,33 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css, return 0; } +static int blkcg_read_appid(struct seq_file *sf, void *v) +{ + struct blkcg *blkcg = css_to_blkcg(seq_css(sf)); + + seq_printf(sf, "%s\n", blkcg->app_identifier); + return 0; +} + +static ssize_t blkcg_write_appid(struct kernfs_open_file *of, + char *buf, size_t nbytes, loff_t off) +{ + struct cgroup_subsys_state *css = of_css(of); + struct blkcg *blkcg = css_to_blkcg(css); + struct blkcg_gq *blkg; + int i; + + buf = strstrip(buf); + if (blkcg) { + if (nbytes < APPID_LEN) + strlcpy(blkcg->app_identifier, buf, nbytes); + else + return -EINVAL; + } + return nbytes; +} + + const char *blkg_dev_name(struct blkcg_gq *blkg) { /* some drivers (floppy) instantiate a queue w/o disk registered */ @@ -844,6 +871,11 @@ static struct cftype blkcg_legacy_files[] = { .name = "reset_stats", .write_u64 = blkcg_reset_stats, }, + { + .name = "app_identifier", + .write = blkcg_write_appid, + .seq_show = blkcg_read_appid, + }, { } /* terminate */ }; diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h index a57ebe2f00ab..3676d7ebb19f 100644 --- a/include/linux/blk-cgroup.h +++ b/include/linux/blk-cgroup.h @@ -30,6 +30,7 @@ /* Max limits for throttle policy */ #define THROTL_IOPS_MAX UINT_MAX +#define APPID_LEN 128 #ifdef CONFIG_BLK_CGROUP @@ -55,6 +56,7 @@ struct blkcg { struct blkcg_policy_data *cpd[BLKCG_MAX_POLS]; struct list_head all_blkcgs_node; + char app_identifier[APPID_LEN]; #ifdef CONFIG_CGROUP_WRITEBACK struct list_head cgwb_list; #endif @@ -239,6 +241,23 @@ static inline struct blkcg *css_to_blkcg(struct cgroup_subsys_state *css) return css ? container_of(css, struct blkcg, css) : NULL; } +/** + * blkcg_get_app_identifier - grab the app identifier associated with a bio + * @bio: target bio + * + * This returns the app identifier associated with a bio, + * %NULL if not associated. + * Callers are expected to either handle %NULL or know association has been + * done prior to calling this. + */ +static inline char *blkcg_get_app_identifier(struct bio *bio) +{ + if (bio && (bio->bi_blkg) && + (strlen(bio->bi_blkg->blkcg->app_identifier))) + return bio->bi_blkg->blkcg->app_identifier; + return NULL; +} + /** * __bio_blkcg - internal, inconsistent version to get blkcg *
This Patch added a unique application identifier i.e blkio.app_identifier knob to blkio controller which allows identification of traffic sources at an individual cgroup based Applications (ex:virtual machine (VM))level in both host and fabric infrastructure. Also provided an interface blkcg_get_app_identifier to grab the app identifier associated with a bio. Added a sysfs interface blkio.app_identifier to get/set the appid. This capability can be utilized by multiple block transport infrastructure like fc,iscsi,roce .. Signed-off-by: Muneendra <muneendra.kumar@broadcom.com> --- block/blk-cgroup.c | 32 ++++++++++++++++++++++++++++++++ include/linux/blk-cgroup.h | 19 +++++++++++++++++++ 2 files changed, 51 insertions(+)