| Message ID | 20250220233310.1342059-1-msakai@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dm vdo indexer: reorder uds_request to reduce padding | expand |
On Thu, 20 Feb 2025, Matthew Sakai wrote: > From: Ken Raeburn <raeburn@redhat.com> > > Reorder fields and make uds_request_type and uds_zone_message packed, > to squeeze out some space. Use struct_group so the request reset code > no longer needs to care about field order. > > On x86_64 this reduces the struct size from 144 to 120, which saves 48 > kB (about 12%) per VDO hash zone. > > Signed-off-by: Ken Raeburn <raeburn@redhat.com> > Signed-off-by: Matthew Sakai <msakai@redhat.com> Hi This patch doesn't apply due to conflicts. Please resend this patch against git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git branch remotes/origin/for-next I accepted the other 3 patches. Mikulas > --- > drivers/md/dm-vdo/indexer/index-session.c | 6 +-- > drivers/md/dm-vdo/indexer/indexer.h | 53 +++++++++++------------ > 2 files changed, 27 insertions(+), 32 deletions(-) > > diff --git a/drivers/md/dm-vdo/indexer/index-session.c b/drivers/md/dm-vdo/indexer/index-session.c > index aee0914d604a..aa575a24e0b2 100644 > --- a/drivers/md/dm-vdo/indexer/index-session.c > +++ b/drivers/md/dm-vdo/indexer/index-session.c > @@ -100,7 +100,6 @@ static int get_index_session(struct uds_index_session *index_session) > > int uds_launch_request(struct uds_request *request) > { > - size_t internal_size; > int result; > > if (request->callback == NULL) { > @@ -121,10 +120,7 @@ int uds_launch_request(struct uds_request *request) > } > > /* Reset all internal fields before processing. */ > - internal_size = > - sizeof(struct uds_request) - offsetof(struct uds_request, zone_number); > - // FIXME should be using struct_group for this instead > - memset((char *) request + sizeof(*request) - internal_size, 0, internal_size); > + memset(&request->internal, 0, sizeof(request->internal)); > > result = get_index_session(request->session); > if (result != UDS_SUCCESS) > diff --git a/drivers/md/dm-vdo/indexer/indexer.h b/drivers/md/dm-vdo/indexer/indexer.h > index 3744aaf625b0..d765f24328eb 100644 > --- a/drivers/md/dm-vdo/indexer/indexer.h > +++ b/drivers/md/dm-vdo/indexer/indexer.h > @@ -8,6 +8,7 @@ > > #include <linux/mutex.h> > #include <linux/sched.h> > +#include <linux/stddef.h> > #include <linux/types.h> > #include <linux/wait.h> > > @@ -73,7 +74,7 @@ enum uds_request_type { > /* Remove any mapping for a name. */ > UDS_DELETE, > > -}; > +} __packed; > > enum uds_open_index_type { > /* Create a new index. */ > @@ -226,7 +227,7 @@ struct uds_zone_message { > enum uds_zone_message_type type; > /* The virtual chapter number to which the message applies */ > u64 virtual_chapter; > -}; > +} __packed; > > struct uds_index_session; > struct uds_index; > @@ -253,34 +254,32 @@ struct uds_request { > > /* The existing data associated with the request name, if any */ > struct uds_record_data old_metadata; > - /* Either UDS_SUCCESS or an error code for the request */ > - int status; > /* True if the record name had an existing entry in the index */ > bool found; > + /* Either UDS_SUCCESS or an error code for the request */ > + int status; > > - /* > - * The remaining fields are used internally and should not be altered by clients. The index > - * relies on zone_number being the first field in this section. > - */ > - > - /* The number of the zone which will process this request*/ > - unsigned int zone_number; > - /* A link for adding a request to a lock-free queue */ > - struct funnel_queue_entry queue_link; > - /* A link for adding a request to a standard linked list */ > - struct uds_request *next_request; > - /* A pointer to the index processing this request */ > - struct uds_index *index; > - /* Control message for coordinating between zones */ > - struct uds_zone_message zone_message; > - /* If true, process request immediately by waking the worker thread */ > - bool unbatched; > - /* If true, continue this request before processing newer requests */ > - bool requeued; > - /* The virtual chapter containing the record name, if known */ > - u64 virtual_chapter; > - /* The region of the index containing the record name */ > - enum uds_index_region location; > + /* The remaining fields are used internally and should not be altered by clients. */ > + struct_group(internal, > + /* The virtual chapter containing the record name, if known */ > + u64 virtual_chapter; > + /* The region of the index containing the record name */ > + enum uds_index_region location; > + /* If true, process request immediately by waking the worker thread */ > + bool unbatched; > + /* If true, continue this request before processing newer requests */ > + bool requeued; > + /* Control message for coordinating between zones */ > + struct uds_zone_message zone_message; > + /* The number of the zone which will process this request*/ > + unsigned int zone_number; > + /* A link for adding a request to a lock-free queue */ > + struct funnel_queue_entry queue_link; > + /* A link for adding a request to a standard linked list */ > + struct uds_request *next_request; > + /* A pointer to the index processing this request */ > + struct uds_index *index; > + ); > }; > > /* Compute the number of bytes needed to store an index. */ > -- > 2.45.2 > >
On 2/24/25 6:33 AM, Mikulas Patocka wrote: > > > On Thu, 20 Feb 2025, Matthew Sakai wrote: > >> From: Ken Raeburn <raeburn@redhat.com> >> >> Reorder fields and make uds_request_type and uds_zone_message packed, >> to squeeze out some space. Use struct_group so the request reset code >> no longer needs to care about field order. >> >> On x86_64 this reduces the struct size from 144 to 120, which saves 48 >> kB (about 12%) per VDO hash zone. >> >> Signed-off-by: Ken Raeburn <raeburn@redhat.com> >> Signed-off-by: Matthew Sakai <msakai@redhat.com> > > Hi > > This patch doesn't apply due to conflicts. Please resend this patch > against > git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git > branch remotes/origin/for-next Sorry about that, I'll send a new version. > I accepted the other 3 patches. > > Mikulas > >> --- >> drivers/md/dm-vdo/indexer/index-session.c | 6 +-- >> drivers/md/dm-vdo/indexer/indexer.h | 53 +++++++++++------------ >> 2 files changed, 27 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/md/dm-vdo/indexer/index-session.c b/drivers/md/dm-vdo/indexer/index-session.c >> index aee0914d604a..aa575a24e0b2 100644 >> --- a/drivers/md/dm-vdo/indexer/index-session.c >> +++ b/drivers/md/dm-vdo/indexer/index-session.c >> @@ -100,7 +100,6 @@ static int get_index_session(struct uds_index_session *index_session) >> >> int uds_launch_request(struct uds_request *request) >> { >> - size_t internal_size; >> int result; >> >> if (request->callback == NULL) { >> @@ -121,10 +120,7 @@ int uds_launch_request(struct uds_request *request) >> } >> >> /* Reset all internal fields before processing. */ >> - internal_size = >> - sizeof(struct uds_request) - offsetof(struct uds_request, zone_number); >> - // FIXME should be using struct_group for this instead >> - memset((char *) request + sizeof(*request) - internal_size, 0, internal_size); >> + memset(&request->internal, 0, sizeof(request->internal)); >> >> result = get_index_session(request->session); >> if (result != UDS_SUCCESS) >> diff --git a/drivers/md/dm-vdo/indexer/indexer.h b/drivers/md/dm-vdo/indexer/indexer.h >> index 3744aaf625b0..d765f24328eb 100644 >> --- a/drivers/md/dm-vdo/indexer/indexer.h >> +++ b/drivers/md/dm-vdo/indexer/indexer.h >> @@ -8,6 +8,7 @@ >> >> #include <linux/mutex.h> >> #include <linux/sched.h> >> +#include <linux/stddef.h> >> #include <linux/types.h> >> #include <linux/wait.h> >> >> @@ -73,7 +74,7 @@ enum uds_request_type { >> /* Remove any mapping for a name. */ >> UDS_DELETE, >> >> -}; >> +} __packed; >> >> enum uds_open_index_type { >> /* Create a new index. */ >> @@ -226,7 +227,7 @@ struct uds_zone_message { >> enum uds_zone_message_type type; >> /* The virtual chapter number to which the message applies */ >> u64 virtual_chapter; >> -}; >> +} __packed; >> >> struct uds_index_session; >> struct uds_index; >> @@ -253,34 +254,32 @@ struct uds_request { >> >> /* The existing data associated with the request name, if any */ >> struct uds_record_data old_metadata; >> - /* Either UDS_SUCCESS or an error code for the request */ >> - int status; >> /* True if the record name had an existing entry in the index */ >> bool found; >> + /* Either UDS_SUCCESS or an error code for the request */ >> + int status; >> >> - /* >> - * The remaining fields are used internally and should not be altered by clients. The index >> - * relies on zone_number being the first field in this section. >> - */ >> - >> - /* The number of the zone which will process this request*/ >> - unsigned int zone_number; >> - /* A link for adding a request to a lock-free queue */ >> - struct funnel_queue_entry queue_link; >> - /* A link for adding a request to a standard linked list */ >> - struct uds_request *next_request; >> - /* A pointer to the index processing this request */ >> - struct uds_index *index; >> - /* Control message for coordinating between zones */ >> - struct uds_zone_message zone_message; >> - /* If true, process request immediately by waking the worker thread */ >> - bool unbatched; >> - /* If true, continue this request before processing newer requests */ >> - bool requeued; >> - /* The virtual chapter containing the record name, if known */ >> - u64 virtual_chapter; >> - /* The region of the index containing the record name */ >> - enum uds_index_region location; >> + /* The remaining fields are used internally and should not be altered by clients. */ >> + struct_group(internal, >> + /* The virtual chapter containing the record name, if known */ >> + u64 virtual_chapter; >> + /* The region of the index containing the record name */ >> + enum uds_index_region location; >> + /* If true, process request immediately by waking the worker thread */ >> + bool unbatched; >> + /* If true, continue this request before processing newer requests */ >> + bool requeued; >> + /* Control message for coordinating between zones */ >> + struct uds_zone_message zone_message; >> + /* The number of the zone which will process this request*/ >> + unsigned int zone_number; >> + /* A link for adding a request to a lock-free queue */ >> + struct funnel_queue_entry queue_link; >> + /* A link for adding a request to a standard linked list */ >> + struct uds_request *next_request; >> + /* A pointer to the index processing this request */ >> + struct uds_index *index; >> + ); >> }; >> >> /* Compute the number of bytes needed to store an index. */ >> -- >> 2.45.2 >> >> >
diff --git a/drivers/md/dm-vdo/indexer/index-session.c b/drivers/md/dm-vdo/indexer/index-session.c index aee0914d604a..aa575a24e0b2 100644 --- a/drivers/md/dm-vdo/indexer/index-session.c +++ b/drivers/md/dm-vdo/indexer/index-session.c @@ -100,7 +100,6 @@ static int get_index_session(struct uds_index_session *index_session) int uds_launch_request(struct uds_request *request) { - size_t internal_size; int result; if (request->callback == NULL) { @@ -121,10 +120,7 @@ int uds_launch_request(struct uds_request *request) } /* Reset all internal fields before processing. */ - internal_size = - sizeof(struct uds_request) - offsetof(struct uds_request, zone_number); - // FIXME should be using struct_group for this instead - memset((char *) request + sizeof(*request) - internal_size, 0, internal_size); + memset(&request->internal, 0, sizeof(request->internal)); result = get_index_session(request->session); if (result != UDS_SUCCESS) diff --git a/drivers/md/dm-vdo/indexer/indexer.h b/drivers/md/dm-vdo/indexer/indexer.h index 3744aaf625b0..d765f24328eb 100644 --- a/drivers/md/dm-vdo/indexer/indexer.h +++ b/drivers/md/dm-vdo/indexer/indexer.h @@ -8,6 +8,7 @@ #include <linux/mutex.h> #include <linux/sched.h> +#include <linux/stddef.h> #include <linux/types.h> #include <linux/wait.h> @@ -73,7 +74,7 @@ enum uds_request_type { /* Remove any mapping for a name. */ UDS_DELETE, -}; +} __packed; enum uds_open_index_type { /* Create a new index. */ @@ -226,7 +227,7 @@ struct uds_zone_message { enum uds_zone_message_type type; /* The virtual chapter number to which the message applies */ u64 virtual_chapter; -}; +} __packed; struct uds_index_session; struct uds_index; @@ -253,34 +254,32 @@ struct uds_request { /* The existing data associated with the request name, if any */ struct uds_record_data old_metadata; - /* Either UDS_SUCCESS or an error code for the request */ - int status; /* True if the record name had an existing entry in the index */ bool found; + /* Either UDS_SUCCESS or an error code for the request */ + int status; - /* - * The remaining fields are used internally and should not be altered by clients. The index - * relies on zone_number being the first field in this section. - */ - - /* The number of the zone which will process this request*/ - unsigned int zone_number; - /* A link for adding a request to a lock-free queue */ - struct funnel_queue_entry queue_link; - /* A link for adding a request to a standard linked list */ - struct uds_request *next_request; - /* A pointer to the index processing this request */ - struct uds_index *index; - /* Control message for coordinating between zones */ - struct uds_zone_message zone_message; - /* If true, process request immediately by waking the worker thread */ - bool unbatched; - /* If true, continue this request before processing newer requests */ - bool requeued; - /* The virtual chapter containing the record name, if known */ - u64 virtual_chapter; - /* The region of the index containing the record name */ - enum uds_index_region location; + /* The remaining fields are used internally and should not be altered by clients. */ + struct_group(internal, + /* The virtual chapter containing the record name, if known */ + u64 virtual_chapter; + /* The region of the index containing the record name */ + enum uds_index_region location; + /* If true, process request immediately by waking the worker thread */ + bool unbatched; + /* If true, continue this request before processing newer requests */ + bool requeued; + /* Control message for coordinating between zones */ + struct uds_zone_message zone_message; + /* The number of the zone which will process this request*/ + unsigned int zone_number; + /* A link for adding a request to a lock-free queue */ + struct funnel_queue_entry queue_link; + /* A link for adding a request to a standard linked list */ + struct uds_request *next_request; + /* A pointer to the index processing this request */ + struct uds_index *index; + ); }; /* Compute the number of bytes needed to store an index. */