Hi Julian,
Thanks for the patch! I had a chance to look at it and have some
comments. Overall I think its great. It provides functionality
we've been needing for a while now -- the hints may become a good way
to 'stage' parameters that we can make more permanent later on. I
would like to see this added to the tree quickly, and so while my
comments may seem nit-picky and anal, I want to make sure it fits in
well with the rest of the code.
I think we may need to copy the hint structure within the PVFS_isys_*
calls. We can't guarantee that the caller won't destroy his hints
structure after the non-blocking call completes.
I would like to see the encoding and decoding for the
'transfer_to_server' hints be done in binary instead of a string.
I'm thinking of something along the lines where a static struct would
define encode/decode functions for a request-id hint, that would
encode it into a 32bit value, instead of a string. The decoding
could also just be to a 32bit int. This would require an internal
representation of a hint, different from the exposed string form, but
would save us shipping a bunch of bytes just because the integer
value is large.
Related to that, the hint struct in pvfs2-hint.h doesn't have to be
exposed. Since we're adding hints to the hint list with functions,
we could keep the actual fields of the struct internal. This would
allow us to convert a known hint immediately to some internal form.
Your calculating the size of the hints for each request encode, which
goes against the pre-calculated request size stuff. Maybe it makes
sense at this point to get rid of that pre-calculated request sizes
code, but since its there right now, we should probably follow that
model. That means the max_size_array needs to account for the
maximum number of hints possible, and in lebf_initialize you'll have
to fill in the hints struct with the maximum possible hints so the
size is calculated correctly. Alternatively, if the hint struct is
internal, it could keep track of the number of hints with a separate
field, and you could just set that to the max for the initial max
size calculation.
The PINT_hint_encode/decode calls and PINT_hint_add_environment_hints
should be in an internal header. PINT_hint_calc_size as well.
pvfs2_hint_type should be PVFS_hint_type.
Separate from the patch -- the way MPI hints are converted to PVFS
hints seems kludgy. I would rather move the PVFS_hint_get_type call
into the PVFS_add_hint call. If PVFS doesn't recognize the hint, the
PVFS_add_hint call can return an error -PVFS_ENOENT or something,
which the MPI code would probably just ignore. Maybe the ROMIO folks
have better ideas for that though.
Thanks again,
-sam
On Jun 19, 2007, at 4:11 PM, Julian Martin Kunkel wrote:
Hi,
I attached a patch which implements the hint mechanism for PVFS2
the way we
use it right now.
For people not reading all the messages from the mailing list: We
had a
discussion about hints a couple of month ago. A hint allows to
transfer a
string through the layers to the server and down to Flow/Trove and
parly BMI
(where it makes sense).
I had to stripe quite some functionality to reduce the patch to its
core just
providing the hints (and a handful of really small modifications)
and to
merge the functionality to the recent CVS version (with concurrent
statemachines).
Therefore, I hope I did not break something, basic operations
seem to work. Most the time the patch is really straightforward.
I left the "Request ID" hint and the kernel code modifications to
transfer it.
However, the patch contains a handful redundant variables....
Well, I sent it even if it is not perfect to reopen a potential
discussion
about the future (and hopefully final) implementation ;-)
Best regards,
Julian
<hint.patch>
_______________________________________________
Pvfs2-developers mailing list
Pvfs2-developers@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
_______________________________________________
Pvfs2-developers mailing list
Pvfs2-developers@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
