RE: [PATCH v2 1/2] perf: Fujitsu: Add the Uncore MAC PMU driver

["Yoshihiro Furudera (Fujitsu)" <fj5100bi@fujitsu.com>]

Hi, Jonathan

> > > +static ssize_t cpumask_show(struct device *dev,
> > > +			    struct device_attribute *attr, char *buf) {
> > > +	struct mac_pmu *macpmu = to_mac_pmu(dev_get_drvdata(dev));
> > If as suggested below you used an index to store the cpu rather than a
> > one hot mask, you'd need to create the mask here to print it.
> 
> I'll use the CPU index.
> So I will create a mask and print it.
> 
> > > +
> > > +	return cpumap_print_to_pagebuf(true, buf, &macpmu->cpumask); }
> >
> > ...
> >
> > > +static int fujitsu_mac_pmu_online_cpu(unsigned int cpu, struct
> > > +hlist_node *node) {
> > > +	struct mac_pmu *macpmu = hlist_entry_safe(node, struct mac_pmu,
> > > +node);
> > > +
> > > +	/* If there is not a CPU/PMU association pick this CPU */
> > > +	if (cpumask_empty(&macpmu->cpumask))
> > > +		cpumask_set_cpu(cpu, &macpmu->cpumask);
> > As below. Seems like just storing the CPU index (and using -1 for not
> > yet
> > set) would be simpler than
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int fujitsu_mac_pmu_offline_cpu(unsigned int cpu, struct
> > > +hlist_node *node) {
> > > +	struct mac_pmu *macpmu = hlist_entry_safe(node, struct mac_pmu,
> > node);
> > > +	unsigned int target;
> > > +
> > > +	if (!cpumask_test_and_clear_cpu(cpu, &macpmu->cpumask))
> >
> > If you are only ever going to set one bit in the mask, why not just
> > store the CPU index instead?
> 
> I'll use the CPU index instead.

Sorry, I considered using the CPU index, but while implementing, I realized that
using cpumask makes the process simpler. Therefore, I would like to keep it as it is.

Best Regards,
Yoshihiro Furudera