Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation

[Alex Williamson]

On Fri, 2014-07-11 at 10:29 +0800, ethan zhao wrote:
> On 2014/7/11 10:22, Alex Williamson wrote:
> > On Fri, 2014-07-11 at 10:10 +0800, ethan zhao wrote:
> >> Alex,
> >>      Thanks for your reviewing, when I create the patch order, I thought
> >> about the question you concerned for
> >> quit a while, make every patch be independent to each other as possible
> >> as I could, so we can do bisect when hit
> >> problem.
> >>
> >>      I manage to take more time to figure out better patch order.
> >>
> >>      Thanks,
> >>      Ethan
> >>
> >> On 2014/7/11 9:48, Alex Williamson wrote:
> >>> Since there's no 0th patch, I guess I'll comment here.  This series is
> >>> not bisectable, patch 1 breaks the existing implementation.  I'd
> >>> suggest:
> >>>
> >>> patch 1 - fix i40e
> >>               i40e only could be fixed with new interface, so it couldn't
> >> be the first one.
> > It looks like i40e just has it's own copy of pci_vfs_assigned(), why
> > can't your current patch 4/4 be applied now?
>   Yes, i40e has its local copy of pci_vfs_assigned(),it could be 
> simplified .
> with new interface,in another word, its a user of new interface.

It's a user of the existing interface.  You're missing the point,
abstract all the users of the assigned dev_flags first, then you're free
to fix the implementation of the interface in one shot without breaking
anything.

> >>> patch 2 - create assign/deassign that uses dev_flags
> >>               This will be the first ?
> >>> patch 3 - convert users to new interface
> >>               Have to be the later step.
> >>> patch 4 - convert interface to use atomic_t
> >>               Could it be standalone step ?
> >>
> >>    Let me think about it.
> >>
> >>> IMHO, the assigned flag is a horrible hack and I don't know why drivers
> >>> like xenback need to use it.  KVM needs to use it because it doesn't
> >>> actually have a driver to bind to when a device is assigned, it's happy
> >>> to assign devices without any driver.  Thanks,
> >>>
> >>> Alex
> >>>
> >>>
> >>> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
> >>>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
> >>>> get sum of VFs that assigned to VM, access low level configuration space register and
> >>>> then loop in traversing device tree.
> >>>>
> >>>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
> >>>> pci_sriov, and compose two more helper functions
> >>>>
> >>>> pci_sriov_assign_device(),
> >>>> pci_sriov_deassign_device()
> >>>>
> >>>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
> >>>>
> >>>> Passed building on 3.15.5
> >>>>
> >>>> Signed-off-by: Ethan Zhao <ethan.zhao@xxxxxxxxxx>
> >>>> ---
> >>>>    drivers/pci/iov.c   |   65 ++++++++++++++++++++++++++++----------------------
> >>>>    drivers/pci/pci.h   |    1 +
> >>>>    include/linux/pci.h |    4 +++
> >>>>    3 files changed, 41 insertions(+), 29 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>>> index de7a747..72e267f 100644
> >>>> --- a/drivers/pci/iov.c
> >>>> +++ b/drivers/pci/iov.c
> >>>> @@ -382,6 +382,7 @@ found:
> >>>>    	iov->nres = nres;
> >>>>    	iov->ctrl = ctrl;
> >>>>    	iov->total_VFs = total;
> >>>> +	atomic_set(&iov->VFs_assigned_cnt, 0);
> >>>>    	iov->offset = offset;
> >>>>    	iov->stride = stride;
> >>>>    	iov->pgsz = pgsz;
> >>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> >>>>    EXPORT_SYMBOL_GPL(pci_num_vf);
> >>>>    
> >>>>    /**
> >>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> >>>> - * @dev: the PCI device
> >>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> >>>> + * @dev: the physical PCI device that contains the VFs.
> >>>>     *
> >>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
> >>>> + * Returns number of VFs belonging to this device that are assigned to VM.
> >>>>     * If device is not a physical function returns 0.
> >>>>     */
> >>>>    int pci_vfs_assigned(struct pci_dev *dev)
> >>>>    {
> >>>> -	struct pci_dev *vfdev;
> >>>> -	unsigned int vfs_assigned = 0;
> >>>> -	unsigned short dev_id;
> >>>> -
> >>>> -	/* only search if we are a PF */
> >>>>    	if (!dev->is_physfn)
> >>>>    		return 0;
> >>>> +	if (dev->sriov)
> >>>> +		return atomic_read(&dev->sriov->VFs_assigned_cnt);
> >>>> +	return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>>    
> >>>> -	/*
> >>>> -	 * determine the device ID for the VFs, the vendor ID will be the
> >>>> -	 * same as the PF so there is no need to check for that one
> >>>> -	 */
> >>>> -	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> >>>> -
> >>>> -	/* loop through all the VFs to see if we own any that are assigned */
> >>>> -	vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> >>>> -	while (vfdev) {
> >>>> -		/*
> >>>> -		 * It is considered assigned if it is a virtual function with
> >>>> -		 * our dev as the physical function and the assigned bit is set
> >>>> -		 */
> >>>> -		if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> >>>> -		    (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> >>>> -			vfs_assigned++;
> >>>> -
> >>>> -		vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> >>>> -	}
> >>>> +/**
> >>>> + * pci_sriov_assign_device - assign device to VM
> >>>> + * @pdev: the device to be assigned.
> >>>> + */
> >>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> +	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >>>> +	if (pdev->is_virtfn && !pdev->is_physfn)
> >>>> +		if (pdev->physfn)
> >>>> +			if (pdev->physfn->sriov)
> >>>> +				atomic_inc(&pdev->physfn->sriov->
> >>>> +						VFs_assigned_cnt);
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
> >>>>    
> >>>> -	return vfs_assigned;
> >>>> +/**
> >>>> + * pci_sriov_deassign_device - deasign device from VM
> >>>> + * @pdev: the device to be deassigned.
> >>>> + */
> >>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> +	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> +	if (pdev->is_virtfn && !pdev->is_physfn)
> >>>> +		if (pdev->physfn)
> >>>> +			if (pdev->physfn->sriov)
> >>>> +				atomic_dec(&pdev->physfn->sriov->
> >>>> +						VFs_assigned_cnt);
> >>>>    }
> >>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
> >>>>    
> >>>>    /**
> >>>>     * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >>>> index 6bd0822..d17bda2 100644
> >>>> --- a/drivers/pci/pci.h
> >>>> +++ b/drivers/pci/pci.h
> >>>> @@ -235,6 +235,7 @@ struct pci_sriov {
> >>>>    	u32 pgsz;		/* page size for BAR alignment */
> >>>>    	u8 link;		/* Function Dependency Link */
> >>>>    	u16 driver_max_VFs;	/* max num VFs driver supports */
> >>>> +	atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> >>>>    	struct pci_dev *dev;	/* lowest numbered PF */
> >>>>    	struct pci_dev *self;	/* this PF */
> >>>>    	struct mutex lock;	/* lock for VF bus */
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index aab57b4..5cf6833 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> >>>>    void pci_disable_sriov(struct pci_dev *dev);
> >>>>    int pci_num_vf(struct pci_dev *dev);
> >>>>    int pci_vfs_assigned(struct pci_dev *dev);
> >>>> +void pci_sriov_assign_device(struct pci_dev *dev);
> >>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
> >>>>    int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >>>>    int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >>>>    #else
> >>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> >>>>    static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> >>>>    static inline int pci_vfs_assigned(struct pci_dev *dev)
> >>>>    { return 0; }
> >>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> >>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> >>>>    static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >>>>    { return 0; }
> >>>>    static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >>>
> >
> >
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/