[MODERATED] Re: [PATCH v5 09/11] TAAv5 9

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

On Mon, Oct 07, 2019 at 11:01:56PM -0700, speck for Pawan Gupta wrote:
> > > +static DEFINE_MUTEX(tsx_mutex);
> > 
> > I think I asked this before, but in looking at the code I still can't
> > figure it out.  What exactly is this protecting?
> > 
> > It looks like you want to keep only one "writer" out of the sysfs store
> > function at a time, but:
> > 
> > > +ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
> > > +			const char *buf, size_t count)
> > > +{
> > > +	enum tsx_ctrl_states requested_state;
> > > +	ssize_t ret;
> > > +	bool val;
> > > +
> > > +	ret = kstrtobool(buf, &val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	mutex_lock(&tsx_mutex);
> > > +
> > > +	if (val) {
> > > +		tsx_user_cmd = TSX_USER_CMD_ON;
> > > +		requested_state = TSX_CTRL_ENABLE;
> > > +	} else {
> > > +		tsx_user_cmd = TSX_USER_CMD_OFF;
> > > +		requested_state = TSX_CTRL_DISABLE;
> > > +	}
> > > +
> > > +	/* Current state is same as the reqested state, do nothing */
> > > +	if (tsx_ctrl_state == requested_state)
> > > +		goto exit;
> > > +
> > > +	tsx_ctrl_state = requested_state;
> > > +
> > > +	tsx_update_on_each_cpu(val);
> > > +exit:
> > > +	mutex_unlock(&tsx_mutex);
> > 
> > What I think you want to do is just protect the tsx_update_on_each_cpu()
> > function, right?
> 
> Also I believe below two operations needs to be under a lock. Without
> the lock if there are two writers and one is preempted in between these
> operations there is a possibility that tsx_ctrl_state and TSX hardware
> state could go out of sync.
> 
> 	tsx_ctrl_state = requested_state;
> 
> 	// 1st writer gets preempted here
> 	// 2nd writer flips tsx_ctrl_state and writes to the MSR.
> 	// 1st writer wakes up and only writes to the MSR
> 
> 	tsx_update_on_each_cpu(val);
> 	// tsx_ctrl_state and hardware state would be different here.
> 
> Chances of this happening is rare but still a possibility. The lock
> would prevent such a condition.

Greg, is it not a possible scenario for tsx_ctrl_state and MSR write to
be under the lock.

Thanks,
Pawan