Re: [PATCH v2 1/1] dmaengine: idxd: Add IDXD performance monitor support

["Zanussi, Tom" <tom.zanussi@linux.intel.com>]

Hi Vinod,

On 4/21/2021 1:01 AM, Vinod Koul wrote:
> On 20-04-21, 09:13, Zanussi, Tom wrote:
>> Hi Vinod,
>>
>> On 4/20/2021 6:11 AM, Vinod Koul wrote:
>>> On 03-04-21, 11:45, Tom Zanussi wrote:
>>>
>>>> +config INTEL_IDXD_PERFMON
>>>> +	bool "Intel Data Accelerators performance monitor support"
>>>> +	depends on INTEL_IDXD
>>>> +	default y
>>>
>>> default y..?
>>
>> Will change to n.
> 
> That is the default, you may drop this line
> 

OK, will do.

>>
>>>
>>>>    /* IDXD software descriptor */
>>>> @@ -369,4 +399,19 @@ int idxd_cdev_get_major(struct idxd_device *idxd);
>>>>    int idxd_wq_add_cdev(struct idxd_wq *wq);
>>>>    void idxd_wq_del_cdev(struct idxd_wq *wq);
>>>> +/* perfmon */
>>>> +#ifdef CONFIG_INTEL_IDXD_PERFMON
>>>
>>> maybe use IS_ENABLED()
> 
> ?
> 

Yes, I'll change to this.

>>>
>>>> @@ -556,6 +562,8 @@ static int __init idxd_init_module(void)
>>>>    	for (i = 0; i < IDXD_TYPE_MAX; i++)
>>>>    		idr_init(&idxd_idrs[i]);
>>>> +	perfmon_init();
>>>> +
>>>>    	err = idxd_register_bus_type();
>>>>    	if (err < 0)
>>>>    		return err;
>>>> @@ -589,5 +597,6 @@ static void __exit idxd_exit_module(void)
>>>>    	pci_unregister_driver(&idxd_pci_driver);
>>>>    	idxd_cdev_remove();
>>>>    	idxd_unregister_bus_type();
>>>> +	perfmon_exit();
>>>
>>> Ideally would make sense to add perfmon module first and then add use in
>>> idxd..
>>>
>>
>> OK, I'll separate this out into a separate patch.
>>
>>>> +static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr,
>>>> +			    char *buf);
>>>> +
>>>> +static cpumask_t		perfmon_dsa_cpu_mask;
>>>> +static bool			cpuhp_set_up;
>>>> +static enum cpuhp_state		cpuhp_slot;
>>>> +
>>>> +static DEVICE_ATTR_RO(cpumask);
>>>
>>> Pls document these new attributes added
> 
> ?
> 

Yes, I'll add comments to all the attributes (also I'm assuming they don't need to be documented elsewhere).

>>>
>>>> +static int perfmon_collect_events(struct idxd_pmu *idxd_pmu,
>>>> +				  struct perf_event *leader,
>>>> +				  bool dogrp)
>>>
>>> dogrp..?
>>>
>>
>> Yeah, bad name, first thought on seeing it is always 'dog'. ;-)
> 
> Yep, that was my first read as well... i guess it would be better as
> do_grp
> 

Yep, will make change it to that.

Thanks,

Tom