With no users of hv_pkt_iter_next_raw() and no "external" users of
hv_pkt_iter_first_raw(), the iterator functions can be refactored
and simplified to remove some indirection/code.
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
---
drivers/hv/ring_buffer.c | 11 +++++------
include/linux/hyperv.h | 35 ++++-------------------------------
2 files changed, 9 insertions(+), 37 deletions(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 3d215d9dec433..c9357dae2a2c8 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
memcpy(buffer, (const char *)desc + offset, packetlen);
/* Advance ring index to next packet descriptor */
- __hv_pkt_iter_next(channel, desc, true);
+ __hv_pkt_iter_next(channel, desc);
/* Notify host of update */
hv_pkt_iter_close(channel);
@@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
/*
* Get first vmbus packet without copying it out of the ring buffer
*/
-struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
+static struct vmpacket_descriptor *
+hv_pkt_iter_first_raw(struct vmbus_channel *channel)
{
struct hv_ring_buffer_info *rbi = &channel->inbound;
@@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);
}
-EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
/*
* Get first vmbus packet from ring buffer after read_index
@@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
*/
struct vmpacket_descriptor *
__hv_pkt_iter_next(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *desc,
- bool copy)
+ const struct vmpacket_descriptor *desc)
{
struct hv_ring_buffer_info *rbi = &channel->inbound;
u32 packetlen = desc->len8 << 3;
@@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
rbi->priv_read_index -= dsize;
/* more data? */
- return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
+ return hv_pkt_iter_first(channel);
}
EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 1112c5cf894e6..370adc9971d3e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc)
return desc->len8 << 3;
}
-struct vmpacket_descriptor *
-hv_pkt_iter_first_raw(struct vmbus_channel *channel);
-
struct vmpacket_descriptor *
hv_pkt_iter_first(struct vmbus_channel *channel);
struct vmpacket_descriptor *
__hv_pkt_iter_next(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt,
- bool copy);
+ const struct vmpacket_descriptor *pkt);
void hv_pkt_iter_close(struct vmbus_channel *channel);
static inline struct vmpacket_descriptor *
-hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt,
- bool copy)
+hv_pkt_iter_next(struct vmbus_channel *channel,
+ const struct vmpacket_descriptor *pkt)
{
struct vmpacket_descriptor *nxt;
- nxt = __hv_pkt_iter_next(channel, pkt, copy);
+ nxt = __hv_pkt_iter_next(channel, pkt);
if (!nxt)
hv_pkt_iter_close(channel);
return nxt;
}
-/*
- * Get next packet descriptor without copying it out of the ring buffer
- * If at end of list, return NULL and update host.
- */
-static inline struct vmpacket_descriptor *
-hv_pkt_iter_next_raw(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt)
-{
- return hv_pkt_iter_next_pkt(channel, pkt, false);
-}
-
-/*
- * Get next packet descriptor from iterator
- * If at end of list, return NULL and update host.
- */
-static inline struct vmpacket_descriptor *
-hv_pkt_iter_next(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt)
-{
- return hv_pkt_iter_next_pkt(channel, pkt, true);
-}
-
#define foreach_vmbus_pkt(pkt, channel) \
for (pkt = hv_pkt_iter_first(channel); pkt; \
pkt = hv_pkt_iter_next(channel, pkt))
--
2.25.1
On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > vmbus_channel *channel)
> > >
> > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > >priv_read_index);
> > > }
> > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> >
> > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > separate function? I think after these changes, the only caller
> > is hv_pkt_iter_first(), in which case the code could just go
> > inline in hv_pkt_iter_first(). Doing that combining would
> > also allow the elimination of the duplicate call to
> > hv_pkt_iter_avail().
Back to this, can you clarify what you mean by "the elimination of..."?
After moving the function "inline", hv_pkt_iter_avail() would be called
in to check for a non-NULL descriptor (in the inline function) and later
in the computation of bytes_avail.
Thanks,
Andrea
>
> Good point. Will do.
>
> Thanks,
> Andrea
On Fri, Apr 15, 2022 at 04:44:50PM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri <[email protected]> Sent: Friday, April 15, 2022 9:28 AM
> >
> > On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > > > vmbus_channel *channel)
> > > > >
> > > > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > > > >priv_read_index);
> > > > > }
> > > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> > > >
> > > > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > > > separate function? I think after these changes, the only caller
> > > > is hv_pkt_iter_first(), in which case the code could just go
> > > > inline in hv_pkt_iter_first(). Doing that combining would
> > > > also allow the elimination of the duplicate call to
> > > > hv_pkt_iter_avail().
> >
> > Back to this, can you clarify what you mean by "the elimination of..."?
> > After moving the function "inline", hv_pkt_iter_avail() would be called
> > in to check for a non-NULL descriptor (in the inline function) and later
> > in the computation of bytes_avail.
>
> I was thinking something like this:
>
> bytes_avail = hv_pkt_iter_avail(rbi);
> if (bytes_avail < sizeof(struct vmpacket_descriptor))
> return NULL;
> bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
>
> desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);
Thanks for the clarification, I've applied it.
Andrea
> And for that matter, hv_pkt_iter_avail() is now only called in one place.
> It's a judgment call whether to keep it as a separate helper function vs.
> inlining it in hv_pkt_iter_first() as well. I'm OK either way.
>
>
> Michael
>
>
From: Andrea Parri <[email protected]> Sent: Friday, April 15, 2022 9:28 AM
>
> On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > > vmbus_channel *channel)
> > > >
> > > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > > >priv_read_index);
> > > > }
> > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> > >
> > > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > > separate function? I think after these changes, the only caller
> > > is hv_pkt_iter_first(), in which case the code could just go
> > > inline in hv_pkt_iter_first(). Doing that combining would
> > > also allow the elimination of the duplicate call to
> > > hv_pkt_iter_avail().
>
> Back to this, can you clarify what you mean by "the elimination of..."?
> After moving the function "inline", hv_pkt_iter_avail() would be called
> in to check for a non-NULL descriptor (in the inline function) and later
> in the computation of bytes_avail.
I was thinking something like this:
bytes_avail = hv_pkt_iter_avail(rbi);
if (bytes_avail < sizeof(struct vmpacket_descriptor))
return NULL;
bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);
And for that matter, hv_pkt_iter_avail() is now only called in one place.
It's a judgment call whether to keep it as a separate helper function vs.
inlining it in hv_pkt_iter_first() as well. I'm OK either way.
Michael
From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 13, 2022 1:48 PM
>
> With no users of hv_pkt_iter_next_raw() and no "external" users of
> hv_pkt_iter_first_raw(), the iterator functions can be refactored
> and simplified to remove some indirection/code.
>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> ---
> drivers/hv/ring_buffer.c | 11 +++++------
> include/linux/hyperv.h | 35 ++++-------------------------------
> 2 files changed, 9 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 3d215d9dec433..c9357dae2a2c8 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
> memcpy(buffer, (const char *)desc + offset, packetlen);
>
> /* Advance ring index to next packet descriptor */
> - __hv_pkt_iter_next(channel, desc, true);
> + __hv_pkt_iter_next(channel, desc);
>
> /* Notify host of update */
> hv_pkt_iter_close(channel);
> @@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info
> *rbi)
> /*
> * Get first vmbus packet without copying it out of the ring buffer
> */
> -struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
> +static struct vmpacket_descriptor *
> +hv_pkt_iter_first_raw(struct vmbus_channel *channel)
> {
> struct hv_ring_buffer_info *rbi = &channel->inbound;
>
> @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> vmbus_channel *channel)
>
> return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> >priv_read_index);
> }
> -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
Does hv_pkt_iter_first_raw() need to be retained at all as a
separate function? I think after these changes, the only caller
is hv_pkt_iter_first(), in which case the code could just go
inline in hv_pkt_iter_first(). Doing that combining would
also allow the elimination of the duplicate call to
hv_pkt_iter_avail().
Michael
>
> /*
> * Get first vmbus packet from ring buffer after read_index
> @@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
> */
> struct vmpacket_descriptor *
> __hv_pkt_iter_next(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *desc,
> - bool copy)
> + const struct vmpacket_descriptor *desc)
> {
> struct hv_ring_buffer_info *rbi = &channel->inbound;
> u32 packetlen = desc->len8 << 3;
> @@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
> rbi->priv_read_index -= dsize;
>
> /* more data? */
> - return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
> + return hv_pkt_iter_first(channel);
> }
> EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
>
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 1112c5cf894e6..370adc9971d3e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct
> vmpacket_descriptor *desc)
> return desc->len8 << 3;
> }
>
> -struct vmpacket_descriptor *
> -hv_pkt_iter_first_raw(struct vmbus_channel *channel);
> -
> struct vmpacket_descriptor *
> hv_pkt_iter_first(struct vmbus_channel *channel);
>
> struct vmpacket_descriptor *
> __hv_pkt_iter_next(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt,
> - bool copy);
> + const struct vmpacket_descriptor *pkt);
>
> void hv_pkt_iter_close(struct vmbus_channel *channel);
>
> static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt,
> - bool copy)
> +hv_pkt_iter_next(struct vmbus_channel *channel,
> + const struct vmpacket_descriptor *pkt)
> {
> struct vmpacket_descriptor *nxt;
>
> - nxt = __hv_pkt_iter_next(channel, pkt, copy);
> + nxt = __hv_pkt_iter_next(channel, pkt);
> if (!nxt)
> hv_pkt_iter_close(channel);
>
> return nxt;
> }
>
> -/*
> - * Get next packet descriptor without copying it out of the ring buffer
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_raw(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt)
> -{
> - return hv_pkt_iter_next_pkt(channel, pkt, false);
> -}
> -
> -/*
> - * Get next packet descriptor from iterator
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt)
> -{
> - return hv_pkt_iter_next_pkt(channel, pkt, true);
> -}
> -
> #define foreach_vmbus_pkt(pkt, channel) \
> for (pkt = hv_pkt_iter_first(channel); pkt; \
> pkt = hv_pkt_iter_next(channel, pkt))
> --
> 2.25.1
> > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > vmbus_channel *channel)
> >
> > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > >priv_read_index);
> > }
> > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
>
> Does hv_pkt_iter_first_raw() need to be retained at all as a
> separate function? I think after these changes, the only caller
> is hv_pkt_iter_first(), in which case the code could just go
> inline in hv_pkt_iter_first(). Doing that combining would
> also allow the elimination of the duplicate call to
> hv_pkt_iter_avail().
Good point. Will do.
Thanks,
Andrea