| Message ID | 1577736814-21112-2-git-send-email-haiyangz@microsoft.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add vmbus dev_num and enable netvsc async probing | expand |
From: Haiyang Zhang <haiyangz@microsoft.com> Sent: Monday, December 30, 2019 12:14 PM > > This number is set to the first available number, starting from zero, > when a vmbus device's primary channel is offered. Let's use "VMBus" as the capitalization in text. > It will be used for stable naming when Async probing is used. > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > --- > Changes > V2: > Use nest loops in hv_set_devnum, instead of goto. > > drivers/hv/channel_mgmt.c | 38 ++++++++++++++++++++++++++++++++++++-- > include/linux/hyperv.h | 6 ++++++ > 2 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > index 8eb1675..00fa2db 100644 > --- a/drivers/hv/channel_mgmt.c > +++ b/drivers/hv/channel_mgmt.c > @@ -315,6 +315,8 @@ static struct vmbus_channel *alloc_channel(void) > if (!channel) > return NULL; > > + channel->dev_num = HV_DEV_NUM_INVALID; > + > spin_lock_init(&channel->lock); > init_completion(&channel->rescind_event); > > @@ -541,6 +543,36 @@ static void vmbus_add_channel_work(struct work_struct *work) > } > > /* > + * Get the first available device number of its type, then > + * record it in the channel structure. > + */ > +static void hv_set_devnum(struct vmbus_channel *newchannel) > +{ > + struct vmbus_channel *channel; > + int i = -1; > + bool found; > + > + BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex)); > + > + do { > + i++; > + found = false; > + > + list_for_each_entry(channel, &vmbus_connection.chn_list, > + listentry) { > + if (i == channel->dev_num && > + guid_equal(&channel->offermsg.offer.if_type, > + &newchannel->offermsg.offer.if_type)) { > + found = true; > + break; > + } > + } > + } while (found); > + > + newchannel->dev_num = i; > +} > + It took me a little while to figure out what the above algorithm is doing. Perhaps it would help to rename the "found" variable to "in_use", and add this comment before the start of the "do" loop: Iterate through each possible device number starting at zero. If the device number is already in use for a device of this type, try the next device number until finding one that is not in use. This approach selects the smallest device number that is not in use, and so reuses any numbers that are freed by devices that have been removed. > +/* > * vmbus_process_offer - Process the offer by creating a channel/device > * associated with this offer > */ > @@ -573,10 +605,12 @@ static void vmbus_process_offer(struct vmbus_channel > *newchannel) > } > } > > - if (fnew) > + if (fnew) { > + hv_set_devnum(newchannel); > + > list_add_tail(&newchannel->listentry, > &vmbus_connection.chn_list); > - else { > + } else { > /* > * Check to see if this is a valid sub-channel. > */ > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 26f3aee..4f110c5 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -718,6 +718,8 @@ struct vmbus_device { > bool perf_device; > }; > > +#define HV_DEV_NUM_INVALID (-1) > + > struct vmbus_channel { > struct list_head listentry; > > @@ -849,6 +851,10 @@ struct vmbus_channel { > */ > struct vmbus_channel *primary_channel; > /* > + * Used for device naming based on channel offer sequence. > + */ > + int dev_num; > + /* > * Support per-channel state for use by vmbus drivers. > */ > void *per_channel_state; > -- > 1.8.3.1
> -----Original Message----- > From: Michael Kelley <mikelley@microsoft.com> > Sent: Monday, December 30, 2019 8:35 PM > To: Haiyang Zhang <haiyangz@microsoft.com>; sashal@kernel.org; linux- > hyperv@vger.kernel.org; netdev@vger.kernel.org > Cc: Haiyang Zhang <haiyangz@microsoft.com>; KY Srinivasan > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; > olaf@aepfle.de; vkuznets <vkuznets@redhat.com>; davem@davemloft.net; > linux-kernel@vger.kernel.org > Subject: RE: [PATCH V2,net-next, 1/3] Drivers: hv: vmbus: Add a dev_num > variable based on channel offer sequence > > From: Haiyang Zhang <haiyangz@microsoft.com> Sent: Monday, December 30, > 2019 12:14 PM > > > > This number is set to the first available number, starting from zero, > > when a vmbus device's primary channel is offered. > > Let's use "VMBus" as the capitalization in text. Sure I will. > > > It will be used for stable naming when Async probing is used. > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > --- > > Changes > > V2: > > Use nest loops in hv_set_devnum, instead of goto. > > > > drivers/hv/channel_mgmt.c | 38 > ++++++++++++++++++++++++++++++++++++-- > > include/linux/hyperv.h | 6 ++++++ > > 2 files changed, 42 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > > index 8eb1675..00fa2db 100644 > > --- a/drivers/hv/channel_mgmt.c > > +++ b/drivers/hv/channel_mgmt.c > > @@ -315,6 +315,8 @@ static struct vmbus_channel *alloc_channel(void) > > if (!channel) > > return NULL; > > > > + channel->dev_num = HV_DEV_NUM_INVALID; > > + > > spin_lock_init(&channel->lock); > > init_completion(&channel->rescind_event); > > > > @@ -541,6 +543,36 @@ static void vmbus_add_channel_work(struct > work_struct *work) > > } > > > > /* > > + * Get the first available device number of its type, then > > + * record it in the channel structure. > > + */ > > +static void hv_set_devnum(struct vmbus_channel *newchannel) > > +{ > > + struct vmbus_channel *channel; > > + int i = -1; > > + bool found; > > + > > + BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex)); > > + > > + do { > > + i++; > > + found = false; > > + > > + list_for_each_entry(channel, &vmbus_connection.chn_list, > > + listentry) { > > + if (i == channel->dev_num && > > + guid_equal(&channel->offermsg.offer.if_type, > > + &newchannel->offermsg.offer.if_type)) { > > + found = true; > > + break; > > + } > > + } > > + } while (found); > > + > > + newchannel->dev_num = i; > > +} > > + > > It took me a little while to figure out what the above algorithm is doing. > Perhaps it would help to rename the "found" variable to "in_use", and add > this comment before the start of the "do" loop: > > Iterate through each possible device number starting at zero. If the device > number is already in use for a device of this type, try the next device number > until finding one that is not in use. This approach selects the smallest > device number that is not in use, and so reuses any numbers that are freed > by devices that have been removed. I will rename the variable, and add the code comments. Thanks, - Haiyang
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c index 8eb1675..00fa2db 100644 --- a/drivers/hv/channel_mgmt.c +++ b/drivers/hv/channel_mgmt.c @@ -315,6 +315,8 @@ static struct vmbus_channel *alloc_channel(void) if (!channel) return NULL; + channel->dev_num = HV_DEV_NUM_INVALID; + spin_lock_init(&channel->lock); init_completion(&channel->rescind_event); @@ -541,6 +543,36 @@ static void vmbus_add_channel_work(struct work_struct *work) } /* + * Get the first available device number of its type, then + * record it in the channel structure. + */ +static void hv_set_devnum(struct vmbus_channel *newchannel) +{ + struct vmbus_channel *channel; + int i = -1; + bool found; + + BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex)); + + do { + i++; + found = false; + + list_for_each_entry(channel, &vmbus_connection.chn_list, + listentry) { + if (i == channel->dev_num && + guid_equal(&channel->offermsg.offer.if_type, + &newchannel->offermsg.offer.if_type)) { + found = true; + break; + } + } + } while (found); + + newchannel->dev_num = i; +} + +/* * vmbus_process_offer - Process the offer by creating a channel/device * associated with this offer */ @@ -573,10 +605,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) } } - if (fnew) + if (fnew) { + hv_set_devnum(newchannel); + list_add_tail(&newchannel->listentry, &vmbus_connection.chn_list); - else { + } else { /* * Check to see if this is a valid sub-channel. */ diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 26f3aee..4f110c5 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -718,6 +718,8 @@ struct vmbus_device { bool perf_device; }; +#define HV_DEV_NUM_INVALID (-1) + struct vmbus_channel { struct list_head listentry; @@ -849,6 +851,10 @@ struct vmbus_channel { */ struct vmbus_channel *primary_channel; /* + * Used for device naming based on channel offer sequence. + */ + int dev_num; + /* * Support per-channel state for use by vmbus drivers. */ void *per_channel_state;
This number is set to the first available number, starting from zero, when a vmbus device’s primary channel is offered. It will be used for stable naming when Async probing is used. Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> --- Changes V2: Use nest loops in hv_set_devnum, instead of goto. drivers/hv/channel_mgmt.c | 38 ++++++++++++++++++++++++++++++++++++-- include/linux/hyperv.h | 6 ++++++ 2 files changed, 42 insertions(+), 2 deletions(-)