[RFC,01/16] blkcg:Introduce blkio.app_identifier knob to blkio controller
diff mbox series

Message ID 1596507196-27417-2-git-send-email-muneendra.kumar@broadcom.com
State New
Headers show
Series
  • Application specific identification support
Related show

Commit Message

Muneendra Aug. 4, 2020, 2:13 a.m. UTC
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(+)

Comments

Daniel Wagner Aug. 4, 2020, 11:31 a.m. UTC | #1
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
Tejun Heo Aug. 4, 2020, 2:21 p.m. UTC | #2
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.
James Smart Aug. 5, 2020, 12:39 a.m. UTC | #3
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
Ming Lei Aug. 5, 2020, 3:59 a.m. UTC | #4
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
Hannes Reinecke Aug. 5, 2020, 6:33 a.m. UTC | #5
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
Tejun Heo Aug. 5, 2020, 2:39 p.m. UTC | #6
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.
Muneendra Aug. 5, 2020, 5:14 p.m. UTC | #7
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
Tejun Heo Aug. 5, 2020, 5:31 p.m. UTC | #8
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.
Ming Lei Aug. 6, 2020, 2:22 a.m. UTC | #9
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
Muneendra Aug. 6, 2020, 12:31 p.m. UTC | #10
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
Paolo Bonzini Aug. 6, 2020, 1:41 p.m. UTC | #11
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

Patch
diff mbox series

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
  *